[PATCH 0/5] Add Broadcom Kona PWM Support

[PATCH 0/5] Add Broadcom Kona PWM Support

[Tim Kryger]

This series introduces the driver for the Kona PWM controller found in
Broadcom mobile SoCs like bcm281xx and updates the device tree and the
defconfig to enable use of this hardware on the bcm28155 AP board.

It depends on:  https://lkml.org/lkml/2013/11/14/521

Tim Kryger (5):
  Documentation: dt: Add kona-pwm binding
  pwm: kona: Introduce Kona PWM controller support
  ARM: dts: Declare the PWM for bcm11351 (bcm281xx)
  ARM: dts: Enable the PWM for bcm28155 AP board
  ARM: bcm_defconfig: Enable PWM and Backlight

 .../devicetree/bindings/pwm/bcm-kona-pwm.txt       |  24 +++
 arch/arm/boot/dts/bcm11351.dtsi                    |   8 +
 arch/arm/boot/dts/bcm28155-ap.dts                  |   4 +
 arch/arm/configs/bcm_defconfig                     |   2 +
 drivers/pwm/Kconfig                                |  10 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-bcm-kona.c                         | 226 +++++++++++++++++++++
 7 files changed, 275 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
 create mode 100644 drivers/pwm/pwm-bcm-kona.c

-- 
1.8.0.1

[PATCH 1/5] Documentation: dt: Add kona-pwm binding

[Tim Kryger]

Add the binding description for the kona-pwm block found on Broadcom's
mobile SoCs.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Alex Elder <elder@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
---
 .../devicetree/bindings/pwm/bcm-kona-pwm.txt       | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt b/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
new file mode 100644
index 0000000..5c3ea1b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
@@ -0,0 +1,24 @@
+Broadcom's PWM Controller Device Tree bindings
+
+Broadcom's Kona PWM Controller has 6 channels
+
+Required Properties :
+- compatible: should be "brcm,kona-pwm"
+- reg: physical base address and length of the controller's registers
+- clocks: clock specifier for the kona pwm external clock
+- #pwm-cells: should be 2.  The first cell specifies the per-chip index
+  of the PWM to use and the second cell is the period in nanoseconds.
+
+Refer to pwm/pwm.txt for generic pwm controller node properties.
+
+Refer to clocks/clock-bindings.txt for generic clock consumer
+properties
+
+Example:
+
+pwm: pwm@3e01a000 {
+	compatible = "brcm,bcm11351-pwm", "brcm,kona-pwm";
+	reg = <0x3e01a000 0xc4>;
+	clocks = <&pwm_clk>;
+	#pwm-cells = <2>;
+};
-- 
1.8.0.1

[PATCH 2/5] pwm: kona: Introduce Kona PWM controller support

[Tim Kryger]

Add support for the six-channel Kona PWM controller found on Broadcom
mobile SoCs like bcm281xx.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Alex Elder <elder@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
---
 drivers/pwm/Kconfig        |  10 ++
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-bcm-kona.c | 226 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 237 insertions(+)
 create mode 100644 drivers/pwm/pwm-bcm-kona.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 75840b5..4ed5d8b 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -53,6 +53,16 @@ config PWM_ATMEL_TCB
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-atmel-tcb.
 
+config PWM_BCM_KONA
+	tristate "Kona PWM support"
+	depends on ARCH_BCM_MOBILE
+	default y
+	help
+	  Generic PWM framework driver for Broadcom Kona PWM block.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-bcm-kona.
+
 config PWM_BFIN
 	tristate "Blackfin PWM support"
 	depends on BFIN_GPTIMERS
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 77a8c18..dd96938 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_PWM_SYSFS)		+= sysfs.o
 obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
+obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.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-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
new file mode 100644
index 0000000..cdf30d9
--- /dev/null
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -0,0 +1,226 @@
+/*
+ * Copyright (C) 2013 Broadcom Corporation
+ *
+ * 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 version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define KONA_PWM_CHANNEL_CNT		6
+
+#define PWM_CONTROL_OFFSET		(0x00000000)
+#define PWM_CONTROL_INITIAL		(0x3f3f3f00)
+#define PWMOUT_POLARITY(chan)		(0x1 << (8 + chan))
+#define PWMOUT_ENABLE(chan)		(0x1 << chan)
+
+#define PRESCALE_OFFSET			(0x00000004)
+#define PRESCALE_SHIFT(chan)		(chan << 2)
+#define PRESCALE_MASK(chan)		(~(0x7 << (chan << 2)))
+#define PRESCALE_MIN			(0x00000000)
+#define PRESCALE_MAX			(0x00000007)
+
+#define PERIOD_COUNT_OFFSET(chan)	(0x00000008 + (chan << 3))
+#define PERIOD_COUNT_MIN		(0x00000002)
+#define PERIOD_COUNT_MAX		(0x00ffffff)
+
+#define DUTY_CYCLE_HIGH_OFFSET(chan)	(0x0000000c + (chan << 3))
+#define DUTY_CYCLE_HIGH_MIN		(0x00000000)
+#define DUTY_CYCLE_HIGH_MAX		(0x00ffffff)
+
+struct kona_pwmc {
+	struct pwm_chip chip;
+	void __iomem *base;
+	struct clk *clk;
+};
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, int chan)
+{
+	/* New settings take effect on rising edge of enable  bit */
+	writel(readl(kp->base + PWM_CONTROL_OFFSET) & ~PWMOUT_ENABLE(chan),
+	       kp->base + PWM_CONTROL_OFFSET);
+	writel(readl(kp->base + PWM_CONTROL_OFFSET) | PWMOUT_ENABLE(chan),
+	       kp->base + PWM_CONTROL_OFFSET);
+}
+
+static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
+				int duty_ns, int period_ns)
+{
+	struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
+	u64 val, div, clk_rate;
+	unsigned long prescale = PRESCALE_MIN, pc, dc;
+	int chan = pwm->hwpwm;
+
+	/*
+	 * Find period count, duty count and prescale to suit duty_ns and
+	 * period_ns. This is done according to formulas described below:
+	 *
+	 * period_ns = 10^9 * (PRESCALE + 1) * PC / PWM_CLK_RATE
+	 * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
+	 *
+	 * PC = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1))
+	 * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1))
+	 */
+
+	clk_rate = clk_get_rate(kp->clk);
+	while (1) {
+		div = 1000000000;
+		div *= 1 + prescale;
+		val = clk_rate * period_ns;
+		pc = div64_u64(val, div);
+		val = clk_rate * duty_ns;
+		dc = div64_u64(val, div);
+
+		/* If duty_ns or period_ns are not achievable then return */
+		if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+			return -EINVAL;
+
+		/*
+		 * If pc or dc have crossed their upper limit, then increase
+		 * prescale and recalculate pc and dc.
+		 */
+		if (pc > PERIOD_COUNT_MAX || dc > DUTY_CYCLE_HIGH_MAX) {
+			if (++prescale > PRESCALE_MAX)
+				return -EINVAL;
+			continue;
+		}
+		break;
+	}
+
+	/* Program prescale */
+	writel((readl(kp->base + PRESCALE_OFFSET) & PRESCALE_MASK(chan)) |
+	       prescale << PRESCALE_SHIFT(chan),
+	       kp->base + PRESCALE_OFFSET);
+
+	/* Program period count */
+	writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
+
+	/* Program duty cycle high count */
+	writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+
+	if (test_bit(PWMF_ENABLED, &pwm->flags))
+		kona_pwmc_apply_settings(kp, chan);
+
+	return 0;
+}
+
+static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	return kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
+}
+
+static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
+	int chan = pwm->hwpwm;
+
+	/*
+	 * The PWM hardware lacks a proper way to be disabled so
+	 * we just program zero duty cycle high count instead
+	 */
+
+	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+	kona_pwmc_apply_settings(kp, chan);
+}
+
+static const struct pwm_ops kona_pwm_ops = {
+	.config = kona_pwmc_config,
+	.owner = THIS_MODULE,
+	.enable = kona_pwmc_enable,
+	.disable = kona_pwmc_disable,
+};
+
+static int kona_pwmc_probe(struct platform_device *pdev)
+{
+	struct kona_pwmc *kp;
+	struct resource *res;
+	int ret = 0;
+
+	dev_dbg(&pdev->dev, "bcm_kona_pwmc probe\n");
+
+	kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL);
+	if (kp == NULL)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	kp->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(kp->base))
+		return PTR_ERR(kp->base);
+
+	kp->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(kp->clk)) {
+		dev_err(&pdev->dev, "Clock get failed : Err %d\n", ret);
+		return PTR_ERR(kp->clk);
+	}
+
+	ret = clk_prepare_enable(kp->clk);
+	if (ret < 0)
+		return ret;
+
+	/* Set smooth mode, push/pull, and normal polarity for all channels */
+	writel(PWM_CONTROL_INITIAL, kp->base + PWM_CONTROL_OFFSET);
+
+	dev_set_drvdata(&pdev->dev, kp);
+
+	kp->chip.dev = &pdev->dev;
+	kp->chip.ops = &kona_pwm_ops;
+	kp->chip.base = -1;
+	kp->chip.npwm = KONA_PWM_CHANNEL_CNT;
+
+	ret = pwmchip_add(&kp->chip);
+	if (ret < 0) {
+		clk_disable_unprepare(kp->clk);
+		dev_err(&pdev->dev, "pwmchip_add() failed: Err %d\n", ret);
+	}
+
+	return ret;
+}
+
+static int kona_pwmc_remove(struct platform_device *pdev)
+{
+	struct kona_pwmc *kp = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(kp->clk);
+	return pwmchip_remove(&kp->chip);
+}
+
+static const struct of_device_id bcm_kona_pwmc_dt[] = {
+	{.compatible = "brcm,kona-pwm"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
+
+static struct platform_driver kona_pwmc_driver = {
+
+	.driver = {
+		   .name = "bcm-kona-pwm",
+		   .owner = THIS_MODULE,
+		   .of_match_table = bcm_kona_pwmc_dt,
+		   },
+
+	.probe = kona_pwmc_probe,
+	.remove = kona_pwmc_remove,
+};
+
+module_platform_driver(kona_pwmc_driver);
+
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("Driver for KONA PWMC");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1.0");
-- 
1.8.0.1

[PATCH 3/5] ARM: dts: Declare the PWM for bcm11351 (bcm281xx)

[Tim Kryger]

Add the device tree node for the PWM on bcm11351 SoCs.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Alex Elder <elder@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
---
 arch/arm/boot/dts/bcm11351.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
index 1246885..72ba903 100644
--- a/arch/arm/boot/dts/bcm11351.dtsi
+++ b/arch/arm/boot/dts/bcm11351.dtsi
@@ -146,6 +146,14 @@
 		status = "disabled";
 	};
 
+	pwm: pwm@3e01a000 {
+		compatible = "brcm,bcm11351-pwm", "brcm,kona-pwm";
+		reg = <0x3e01a000 0xcc>;
+		clocks = <&pwm_clk>;
+		#pwm-cells = <2>;
+		status = "disabled";
+	};
+
 	clocks {
 		bsc1_clk: bsc1 {
 			compatible = "fixed-clock";
-- 
1.8.0.1

[PATCH 4/5] ARM: dts: Enable the PWM for bcm28155 AP board

[Tim Kryger]

Mark the PWM as enabled on the bcm28155 AP board.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Alex Elder <elder@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
---
 arch/arm/boot/dts/bcm28155-ap.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/bcm28155-ap.dts b/arch/arm/boot/dts/bcm28155-ap.dts
index 08e47c2..2dfca32 100644
--- a/arch/arm/boot/dts/bcm28155-ap.dts
+++ b/arch/arm/boot/dts/bcm28155-ap.dts
@@ -43,4 +43,8 @@
 		cd-gpios = <&gpio 14 0>;
 		status = "okay";
 	};
+
+	pwm: pwm@3e01a000 {
+		status = "okay";
+	};
 };
-- 
1.8.0.1

[PATCH 5/5] ARM: bcm_defconfig: Enable PWM and Backlight

[Tim Kryger]

Enable PWM drivers and the PWM-based baclight driver.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Alex Elder <elder@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
---
 arch/arm/configs/bcm_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/bcm_defconfig b/arch/arm/configs/bcm_defconfig
index fcea348..04a58f34 100644
--- a/arch/arm/configs/bcm_defconfig
+++ b/arch/arm/configs/bcm_defconfig
@@ -85,6 +85,7 @@ CONFIG_FB=y
 CONFIG_BACKLIGHT_LCD_SUPPORT=y
 CONFIG_LCD_CLASS_DEVICE=y
 CONFIG_BACKLIGHT_CLASS_DEVICE=y
+CONFIG_BACKLIGHT_PWM=y
 # CONFIG_USB_SUPPORT is not set
 CONFIG_MMC=y
 CONFIG_MMC_UNSAFE_RESUME=y
@@ -98,6 +99,7 @@ CONFIG_LEDS_TRIGGERS=y
 CONFIG_LEDS_TRIGGER_TIMER=y
 CONFIG_LEDS_TRIGGER_HEARTBEAT=y
 CONFIG_LEDS_TRIGGER_DEFAULT_ON=y
+CONFIG_PWM=y
 CONFIG_EXT4_FS=y
 CONFIG_EXT4_FS_POSIX_ACL=y
 CONFIG_EXT4_FS_SECURITY=y
-- 
1.8.0.1

Re: [PATCH 1/5] Documentation: dt: Add kona-pwm binding

[Mark Rutland]

On Mon, Nov 18, 2013 at 06:54:57PM +0000, Tim Kryger wrote:
> Add the binding description for the kona-pwm block found on Broadcom's
> mobile SoCs.
> 
> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
> Reviewed-by: Alex Elder <elder@linaro.org>
> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
> ---
>  .../devicetree/bindings/pwm/bcm-kona-pwm.txt       | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt b/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
> new file mode 100644
> index 0000000..5c3ea1b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
> @@ -0,0 +1,24 @@
> +Broadcom's PWM Controller Device Tree bindings
> +
> +Broadcom's Kona PWM Controller has 6 channels
> +
> +Required Properties :
> +- compatible: should be "brcm,kona-pwm"
> +- reg: physical base address and length of the controller's registers
> +- clocks: clock specifier for the kona pwm external clock

Minor nit: phandle + clock-specifier pair

> +- #pwm-cells: should be 2.  The first cell specifies the per-chip index
> +  of the PWM to use and the second cell is the period in nanoseconds.
> +
> +Refer to pwm/pwm.txt for generic pwm controller node properties.
> +
> +Refer to clocks/clock-bindings.txt for generic clock consumer
> +properties
> +
> +Example:
> +
> +pwm: pwm@3e01a000 {
> +	compatible = "brcm,bcm11351-pwm", "brcm,kona-pwm";
> +	reg = <0x3e01a000 0xc4>;
> +	clocks = <&pwm_clk>;
> +	#pwm-cells = <2>;
> +};

Otherwise this looks fine.

Mark.

Re: [PATCH 2/5] pwm: kona: Introduce Kona PWM controller support

[Thierry Reding]

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

On Mon, Nov 18, 2013 at 10:54:58AM -0800, Tim Kryger wrote:
[...]
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
[...]
> +config PWM_BCM_KONA
> +	tristate "Kona PWM support"
> +	depends on ARCH_BCM_MOBILE
> +	default y

Why do you want this to be the default? Shouldn't this be something that
a default configuration selects explicitly?

> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
[...]
>  obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
> +obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.o

'C' < 'F'

> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
[...]
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#define KONA_PWM_CHANNEL_CNT		6

You use this exactly once, so there's no need for this define.

> +#define PWM_CONTROL_OFFSET		(0x00000000)

I'd prefer if you dropped the _OFFSET suffix here.

> +#define PWM_CONTROL_INITIAL		(0x3f3f3f00)

Can this not be expressed as a bitmask of values for the individual
fields.

> +#define PWMOUT_POLARITY(chan)		(0x1 << (8 + chan))

This seems to only account for bits 8-13, what about the others?

> +#define PWMOUT_ENABLE(chan)		(0x1 << chan)

Well, this accounts for bits 0-5, that still leaves 16-21 and 24-29.

Also perhaps PWMOUT_POLARITY and PWMOUT_ENABLE should be defined as
PWM_CONTROL_POLARITY and PWM_CONTROL_ENABLE. That makes it easy to see
which register they are related to.

> +#define PRESCALE_OFFSET			(0x00000004)
> +#define PRESCALE_SHIFT(chan)		(chan << 2)

I'm confused. This allocates 2 bits for each channel...

> +#define PRESCALE_MASK(chan)		(~(0x7 << (chan << 2)))
> +#define PRESCALE_MIN			(0x00000000)
> +#define PRESCALE_MAX			(0x00000007)

... but 0x7 requires at least 3 bits.

> +#define PERIOD_COUNT_OFFSET(chan)	(0x00000008 + (chan << 3))
> +#define PERIOD_COUNT_MIN		(0x00000002)
> +#define PERIOD_COUNT_MAX		(0x00ffffff)

Why PERIOD_COUNT? PERIOD is descriptive enough. Or is this the name as
found in the manual?

> +#define DUTY_CYCLE_HIGH_OFFSET(chan)	(0x0000000c + (chan << 3))
> +#define DUTY_CYCLE_HIGH_MIN		(0x00000000)
> +#define DUTY_CYCLE_HIGH_MAX		(0x00ffffff)

By definition the duty-cycle is where the signal is high. Again, if this
is how the manual names the registers it's fine.

> +struct kona_pwmc {
> +	struct pwm_chip chip;
> +	void __iomem *base;
> +	struct clk *clk;
> +};
> +
> +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, int chan)

unsigned int for "chan"?

> +{
> +	/* New settings take effect on rising edge of enable  bit */
> +	writel(readl(kp->base + PWM_CONTROL_OFFSET) & ~PWMOUT_ENABLE(chan),
> +	       kp->base + PWM_CONTROL_OFFSET);
> +	writel(readl(kp->base + PWM_CONTROL_OFFSET) | PWMOUT_ENABLE(chan),
> +	       kp->base + PWM_CONTROL_OFFSET);

That's too cluttered for my taste. Please make this more explicit:

	value = readl(...);
	value &= ~...;
	writel(value, ...);

	value = readl(...);
	value |= ...;
	writel(value, ...);

> +}
> +
> +static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +				int duty_ns, int period_ns)
> +{
> +	struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
> +	u64 val, div, clk_rate;
> +	unsigned long prescale = PRESCALE_MIN, pc, dc;
> +	int chan = pwm->hwpwm;

pwm->hwpwm is unsigned, so chan should be as well.

> +
> +	/*
> +	 * Find period count, duty count and prescale to suit duty_ns and
> +	 * period_ns. This is done according to formulas described below:
> +	 *
> +	 * period_ns = 10^9 * (PRESCALE + 1) * PC / PWM_CLK_RATE
> +	 * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
> +	 *
> +	 * PC = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1))
> +	 * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1))
> +	 */
> +
> +	clk_rate = clk_get_rate(kp->clk);
> +	while (1) {

Newline between the above two lines please.

> +		div = 1000000000;
> +		div *= 1 + prescale;
> +		val = clk_rate * period_ns;
> +		pc = div64_u64(val, div);
> +		val = clk_rate * duty_ns;
> +		dc = div64_u64(val, div);
> +
> +		/* If duty_ns or period_ns are not achievable then return */
> +		if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
> +			return -EINVAL;
> +
> +		/*
> +		 * If pc or dc have crossed their upper limit, then increase
> +		 * prescale and recalculate pc and dc.
> +		 */
> +		if (pc > PERIOD_COUNT_MAX || dc > DUTY_CYCLE_HIGH_MAX) {
> +			if (++prescale > PRESCALE_MAX)
> +				return -EINVAL;
> +			continue;
> +		}

This looks unintuitive to me, perhaps:

		if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
			break;

		if (++prescale > PRESCALE_MAX)
			return -EINVAL;

?

> +	/* Program prescale */
> +	writel((readl(kp->base + PRESCALE_OFFSET) & PRESCALE_MASK(chan)) |
> +	       prescale << PRESCALE_SHIFT(chan),
> +	       kp->base + PRESCALE_OFFSET);

Again, please split this into separate read/modify/write steps.

> +
> +	/* Program period count */
> +	writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
> +
> +	/* Program duty cycle high count */
> +	writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));

I don't think we need the comments. The register names are fairly
descriptive, so the comments add no value.

> +
> +	if (test_bit(PWMF_ENABLED, &pwm->flags))
> +		kona_pwmc_apply_settings(kp, chan);
> +
> +	return 0;
> +}
> +
> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	return kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
> +}

Why can't this just enable the channel? Why go through all the trouble
of running the whole computations again?

> +
> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
> +	int chan = pwm->hwpwm;
> +
> +	/*
> +	 * The PWM hardware lacks a proper way to be disabled so
> +	 * we just program zero duty cycle high count instead
> +	 */

So clearing the enable bit doesn't disable the PWM channel?

> +
> +	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> +	kona_pwmc_apply_settings(kp, chan);
> +}
> +
> +static const struct pwm_ops kona_pwm_ops = {
> +	.config = kona_pwmc_config,
> +	.owner = THIS_MODULE,
> +	.enable = kona_pwmc_enable,
> +	.disable = kona_pwmc_disable,
> +};

Please move the .owner field to be the last field. Also you did define
the PWMOUT_POLARITY field, which indicates that the hardware supports
changing the signal's polarity, yet you don't implement the polarity
feature. Why not?

> +static int kona_pwmc_probe(struct platform_device *pdev)
> +{
> +	struct kona_pwmc *kp;
> +	struct resource *res;
> +	int ret = 0;

I don't think this needs to be initialized.

> +
> +	dev_dbg(&pdev->dev, "bcm_kona_pwmc probe\n");

Can this be removed?

> +
> +	kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL);
> +	if (kp == NULL)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	kp->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(kp->base))
> +		return PTR_ERR(kp->base);
> +
> +	kp->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(kp->clk)) {
> +		dev_err(&pdev->dev, "Clock get failed : Err %d\n", ret);

ret would be 0 here, indicating no error. This should probably be
PTR_ERR(kp->clk). Also please make the error message more consistent,
this one and the one below use completely different styles. Also, "Err"
isn't very useful in an error message. Something like:

		dev_err(&pdev->dev, "failed to get clock: %d\n",
			PTR_ERR(kp->clk));

would be good.

> +		return PTR_ERR(kp->clk);
> +	}
> +
> +	ret = clk_prepare_enable(kp->clk);
> +	if (ret < 0)
> +		return ret;

Do you really want the clock enabled all the time? Why not just
clk_enable() whenever a PWM is enabled? If you need the clock for
register access, you can also bracket register accesses with
clk_enable() and clk_disable(). Perhaps the power savings aren't worth
the added effort, so if you'd rather not do that, I'm fine with it, too.

> +
> +	/* Set smooth mode, push/pull, and normal polarity for all channels */
> +	writel(PWM_CONTROL_INITIAL, kp->base + PWM_CONTROL_OFFSET);

I'd expect to see bitfield definitions for smooth mode and push/pull,
and PWM_CONTROL_INITIAL to be defined in terms of those. Better yet
would be to have a value constructed at runtime with the initial value.

> +	dev_set_drvdata(&pdev->dev, kp);

platform_set_drvdata(), please.

> +	kp->chip.dev = &pdev->dev;
> +	kp->chip.ops = &kona_pwm_ops;
> +	kp->chip.base = -1;
> +	kp->chip.npwm = KONA_PWM_CHANNEL_CNT;
> +
> +	ret = pwmchip_add(&kp->chip);
> +	if (ret < 0) {
> +		clk_disable_unprepare(kp->clk);
> +		dev_err(&pdev->dev, "pwmchip_add() failed: Err %d\n", ret);

For consistency with my above recommendation:

		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);

Also, I'd move the error message before clk_disable_unprepare(). There's
no technical reason really, but it's far more common that way around.

> +	}
> +
> +	return ret;
> +}
> +
> +static int kona_pwmc_remove(struct platform_device *pdev)
> +{
> +	struct kona_pwmc *kp = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(kp->clk);
> +	return pwmchip_remove(&kp->chip);
> +}
> +
> +static const struct of_device_id bcm_kona_pwmc_dt[] = {
> +	{.compatible = "brcm,kona-pwm"},

Needs spaces after { and before }.

> +	{},

Should be: { }.

> +};
> +MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
> +
> +static struct platform_driver kona_pwmc_driver = {
> +
> +	.driver = {
> +		   .name = "bcm-kona-pwm",
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = bcm_kona_pwmc_dt,
> +		   },

The alignment is weird, should be:

	.driver = {
		.name = "bcm-kona-pwm",
		.owner = THIS_MODULE,
		.of_match_table = bcm_kona_pwmc_dt,
	},

You can also leave out the .owner field, that's assigned automatically
by the driver core.

> +
> +	.probe = kona_pwmc_probe,

No blank line before this one.

> +	.remove = kona_pwmc_remove,
> +};
> +
> +module_platform_driver(kona_pwmc_driver);

No blank line before this one.

> +
> +MODULE_AUTHOR("Broadcom");

I don't think Broadcom qualifies as author. This should be the name of
whoever wrote the code. There are a few drivers that contain the company
name in the MODULE_AUTHOR, but I don't think those are correct either.

> +MODULE_DESCRIPTION("Driver for KONA PWMC");

"Driver for KONA PWM controller"?

> +MODULE_LICENSE("GPL");

According to the header comment this should be "GPL v2".

> +MODULE_VERSION("1.0");

I don't think we need this.

Thierry

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

Re: [PATCH 1/5] Documentation: dt: Add kona-pwm binding

[Thierry Reding]

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

On Mon, Nov 18, 2013 at 10:54:57AM -0800, Tim Kryger wrote:
> Add the binding description for the kona-pwm block found on Broadcom's
> mobile SoCs.
> 
> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
> Reviewed-by: Alex Elder <elder@linaro.org>
> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
> ---
>  .../devicetree/bindings/pwm/bcm-kona-pwm.txt       | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt b/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
> new file mode 100644
> index 0000000..5c3ea1b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
> @@ -0,0 +1,24 @@
> +Broadcom's PWM Controller Device Tree bindings

This sounds somewhat strange, like Broadcom owns the bindings... perhaps
just:

	Broadcom PWM controller device tree bindings

Or for consistency:

	Broadcom Kona PWM controller device tree bindings

since apparently there exist other Broadcom PWM controllers.

> +Broadcom's Kona PWM Controller has 6 channels

"controller" and end the sentence with a full stop.

> +
> +Required Properties :
> +- compatible: should be "brcm,kona-pwm"
> +- reg: physical base address and length of the controller's registers
> +- clocks: clock specifier for the kona pwm external clock

Please use the correct spelling "PWM" in prose here and elsewhere. Also
you should pick one of "kona", "Kona" or "KONA" and stick with it
consistently. Might also be useful to mention what Kona is. Doesn't
necessarily have to be in the document, but if for nothing else, then
just for my curiosity.

> +- #pwm-cells: should be 2.  The first cell specifies the per-chip index
> +  of the PWM to use and the second cell is the period in nanoseconds.
> +
> +Refer to pwm/pwm.txt for generic pwm controller node properties.
> +
> +Refer to clocks/clock-bindings.txt for generic clock consumer
> +properties

This sentence should also end with a full stop.

Thierry

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

Re: [PATCH 2/5] pwm: kona: Introduce Kona PWM controller support

[Tim Kryger]

On Mon, Nov 25, 2013 at 3:08 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Nov 18, 2013 at 10:54:58AM -0800, Tim Kryger wrote:
> [...]
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> [...]
>> +config PWM_BCM_KONA
>> +     tristate "Kona PWM support"
>> +     depends on ARCH_BCM_MOBILE
>> +     default y
>
> Why do you want this to be the default? Shouldn't this be something that
> a default configuration selects explicitly?

This hardware is present on all recent Broadcom mobile SoCs so it is
reasonable to expect that one would want the driver enabled but I
think it makes sense to allow it be compiled out just in case it is
unused.

>> +#define PWM_CONTROL_INITIAL          (0x3f3f3f00)
>
> Can this not be expressed as a bitmask of values for the individual
> fields.
>
>> +#define PWMOUT_POLARITY(chan)                (0x1 << (8 + chan))
>
> This seems to only account for bits 8-13, what about the others?
>
>> +#define PWMOUT_ENABLE(chan)          (0x1 << chan)
>
> Well, this accounts for bits 0-5, that still leaves 16-21 and 24-29.

29:24 - Configures each PWM channel for smooth transitions or glitches
21:16 - Configures each PWM channel for push/pull or open drain output


>> +#define PRESCALE_OFFSET                      (0x00000004)
>> +#define PRESCALE_SHIFT(chan)         (chan << 2)
>
> I'm confused. This allocates 2 bits for each channel...
>
>> +#define PRESCALE_MASK(chan)          (~(0x7 << (chan << 2)))
>> +#define PRESCALE_MIN                 (0x00000000)
>> +#define PRESCALE_MAX                 (0x00000007)
>
> ... but 0x7 requires at least 3 bits.

Actually this is allocating 2^2 bits for each channel.

>> +#define PERIOD_COUNT_OFFSET(chan)    (0x00000008 + (chan << 3))
>> +#define PERIOD_COUNT_MIN             (0x00000002)
>> +#define PERIOD_COUNT_MAX             (0x00ffffff)
>
> Why PERIOD_COUNT? PERIOD is descriptive enough. Or is this the name as
> found in the manual?

I agree but as you suspected this name comes from the hardware docs.

>> +#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + (chan << 3))
>> +#define DUTY_CYCLE_HIGH_MIN          (0x00000000)
>> +#define DUTY_CYCLE_HIGH_MAX          (0x00ffffff)
>
> By definition the duty-cycle is where the signal is high. Again, if this
> is how the manual names the registers it's fine.

Same comment as above.

>> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +     return kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
>> +}
>
> Why can't this just enable the channel? Why go through all the trouble
> of running the whole computations again?

The hardware is always enabled and at best can be be configured to
operate at zero duty.

The settings in HW may have already been triggered and might not be
what you want.

For example:

/sys/class/pwm/pwmchip0/pwm1# echo 1 > enable
/sys/class/pwm/pwmchip0/pwm1# echo 5000 > period
/sys/class/pwm/pwmchip0/pwm1# echo 2000 > duty_cycle
/sys/class/pwm/pwmchip0/pwm1# echo 0 > enable
/sys/class/pwm/pwmchip0/pwm1# echo 1 > enable

>> +
>> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +     struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
>> +     int chan = pwm->hwpwm;
>> +
>> +     /*
>> +      * The PWM hardware lacks a proper way to be disabled so
>> +      * we just program zero duty cycle high count instead
>> +      */
>
> So clearing the enable bit doesn't disable the PWM channel?

That is correct.  They picked a terrible name for this bit.

>> +
>> +     writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
>> +     kona_pwmc_apply_settings(kp, chan);
>> +}
>> +
>> +static const struct pwm_ops kona_pwm_ops = {
>> +     .config = kona_pwmc_config,
>> +     .owner = THIS_MODULE,
>> +     .enable = kona_pwmc_enable,
>> +     .disable = kona_pwmc_disable,
>> +};
>
> Please move the .owner field to be the last field. Also you did define
> the PWMOUT_POLARITY field, which indicates that the hardware supports
> changing the signal's polarity, yet you don't implement the polarity
> feature. Why not?

I wanted to keep this driver simple for now.

>> +     ret = clk_prepare_enable(kp->clk);
>> +     if (ret < 0)
>> +             return ret;
>
> Do you really want the clock enabled all the time? Why not just
> clk_enable() whenever a PWM is enabled? If you need the clock for
> register access, you can also bracket register accesses with
> clk_enable() and clk_disable(). Perhaps the power savings aren't worth
> the added effort, so if you'd rather not do that, I'm fine with it, too.

I intend to follow up with a patch to do exactly that but I want to
establish the baseline functionality first.

>> +MODULE_AUTHOR("Broadcom");
>
> I don't think Broadcom qualifies as author. This should be the name of
> whoever wrote the code. There are a few drivers that contain the company
> name in the MODULE_AUTHOR, but I don't think those are correct either.

Would you be fine with two lines here?  Something like:

MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list@broadcom.com>");
MODULE_AUTHOR("Tim Kryger <tkryger@broadcom.com>");


Thanks for the review.  I will make all of the other changes you have
recommended.

-Tim

Re: [PATCH 1/5] Documentation: dt: Add kona-pwm binding

[Tim Kryger]

On Tue, Nov 19, 2013 at 5:56 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Nov 18, 2013 at 06:54:57PM +0000, Tim Kryger wrote:

>> +Required Properties :
>> +- compatible: should be "brcm,kona-pwm"
>> +- reg: physical base address and length of the controller's registers
>> +- clocks: clock specifier for the kona pwm external clock
>
> Minor nit: phandle + clock-specifier pair

I will incorporate this change into my next version.  Thanks.

-Tim

Re: [PATCH 1/5] Documentation: dt: Add kona-pwm binding

[Tim Kryger]

On Mon, Nov 25, 2013 at 3:17 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Nov 18, 2013 at 10:54:57AM -0800, Tim Kryger wrote:

>> +++ b/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
>> @@ -0,0 +1,24 @@
>> +Broadcom's PWM Controller Device Tree bindings
>
> This sounds somewhat strange, like Broadcom owns the bindings... perhaps
> just:
>
>         Broadcom PWM controller device tree bindings
>
> Or for consistency:
>
>         Broadcom Kona PWM controller device tree bindings

Yes that sounds better.

>> +Broadcom's Kona PWM Controller has 6 channels
>
> "controller" and end the sentence with a full stop.

Sure.

>> +- clocks: clock specifier for the kona pwm external clock
>
> Please use the correct spelling "PWM" in prose here and elsewhere. Also
> you should pick one of "kona", "Kona" or "KONA" and stick with it
> consistently. Might also be useful to mention what Kona is. Doesn't
> necessarily have to be in the document, but if for nothing else, then
> just for my curiosity.

Agreed.  It should be "Kona" here.

Kona roughly defines the set of common IP used on multiple mobile SoCs.

>> +- #pwm-cells: should be 2.  The first cell specifies the per-chip index
>> +  of the PWM to use and the second cell is the period in nanoseconds.
>> +
>> +Refer to pwm/pwm.txt for generic pwm controller node properties.
>> +
>> +Refer to clocks/clock-bindings.txt for generic clock consumer
>> +properties
>
> This sentence should also end with a full stop.

Sure thing.  Thanks again.

-Tim

Re: [PATCH 2/5] pwm: kona: Introduce Kona PWM controller support

[Thierry Reding]

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

On Mon, Nov 25, 2013 at 05:38:44PM -0800, Tim Kryger wrote:
> On Mon, Nov 25, 2013 at 3:08 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Mon, Nov 18, 2013 at 10:54:58AM -0800, Tim Kryger wrote:
> > [...]
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
[...]
> >> +#define PWM_CONTROL_INITIAL          (0x3f3f3f00)
> >
> > Can this not be expressed as a bitmask of values for the individual
> > fields.
> >
> >> +#define PWMOUT_POLARITY(chan)                (0x1 << (8 + chan))
> >
> > This seems to only account for bits 8-13, what about the others?
> >
> >> +#define PWMOUT_ENABLE(chan)          (0x1 << chan)
> >
> > Well, this accounts for bits 0-5, that still leaves 16-21 and 24-29.
> 
> 29:24 - Configures each PWM channel for smooth transitions or glitches
> 21:16 - Configures each PWM channel for push/pull or open drain output

Excellent, now if you can turn that into bit definitions and define
PWM_CONTROL_INITIAL in terms of those, then I'll be very happy.

One other thing I didn't pay attention to before: while it's quite
unlikely ever to happen, you might want to spend an extra pair of
parentheses around "chan", just in case.

> >> +#define PRESCALE_OFFSET                      (0x00000004)
> >> +#define PRESCALE_SHIFT(chan)         (chan << 2)
> >
> > I'm confused. This allocates 2 bits for each channel...
> >
> >> +#define PRESCALE_MASK(chan)          (~(0x7 << (chan << 2)))
> >> +#define PRESCALE_MIN                 (0x00000000)
> >> +#define PRESCALE_MAX                 (0x00000007)
> >
> > ... but 0x7 requires at least 3 bits.
> 
> Actually this is allocating 2^2 bits for each channel.

Doh! Indeed. Perhaps writing it as (~(0x7 << (chan * 4))) would make it
easier to digest for slow-witted people like myself.

> >> +#define PERIOD_COUNT_OFFSET(chan)    (0x00000008 + (chan << 3))
> >> +#define PERIOD_COUNT_MIN             (0x00000002)
> >> +#define PERIOD_COUNT_MAX             (0x00ffffff)
> >
> > Why PERIOD_COUNT? PERIOD is descriptive enough. Or is this the name as
> > found in the manual?
> 
> I agree but as you suspected this name comes from the hardware docs.

Okay, that's fine then. Do you have a pointer to that documentation?

> >> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >> +{
> >> +     return kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
> >> +}
> >
> > Why can't this just enable the channel? Why go through all the trouble
> > of running the whole computations again?
> 
> The hardware is always enabled and at best can be be configured to
> operate at zero duty.

What good are the PWMOUT_ENABLE bits then? Is that really only used for
triggering updates? That's what another comment suggests, but if so, can
the comment in kona_pwmc_apply_settings() be extended to mention that?

> The settings in HW may have already been triggered and might not be
> what you want.
> 
> For example:
> 
> /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable
> /sys/class/pwm/pwmchip0/pwm1# echo 5000 > period
> /sys/class/pwm/pwmchip0/pwm1# echo 2000 > duty_cycle
> /sys/class/pwm/pwmchip0/pwm1# echo 0 > enable
> /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable

I'm not exactly sure what this is supposed to demonstrate.

> >> +
> >> +     writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> >> +     kona_pwmc_apply_settings(kp, chan);
> >> +}
> >> +
> >> +static const struct pwm_ops kona_pwm_ops = {
> >> +     .config = kona_pwmc_config,
> >> +     .owner = THIS_MODULE,
> >> +     .enable = kona_pwmc_enable,
> >> +     .disable = kona_pwmc_disable,
> >> +};
> >
> > Please move the .owner field to be the last field. Also you did define
> > the PWMOUT_POLARITY field, which indicates that the hardware supports
> > changing the signal's polarity, yet you don't implement the polarity
> > feature. Why not?
> 
> I wanted to keep this driver simple for now.

Fair enough.

> > Do you really want the clock enabled all the time? Why not just
> > clk_enable() whenever a PWM is enabled? If you need the clock for
> > register access, you can also bracket register accesses with
> > clk_enable() and clk_disable(). Perhaps the power savings aren't worth
> > the added effort, so if you'd rather not do that, I'm fine with it, too.
> 
> I intend to follow up with a patch to do exactly that but I want to
> establish the baseline functionality first.

Okay, that's fine.

> >> +MODULE_AUTHOR("Broadcom");
> >
> > I don't think Broadcom qualifies as author. This should be the name of
> > whoever wrote the code. There are a few drivers that contain the company
> > name in the MODULE_AUTHOR, but I don't think those are correct either.
> 
> Would you be fine with two lines here?  Something like:
> 
> MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list@broadcom.com>");
> MODULE_AUTHOR("Tim Kryger <tkryger@broadcom.com>");

Not sure. That email address certainly doesn't look like it belongs to
the driver author. Neither did Broadcom actually write the driver, you
did, right? If you're concerned about your employer not being credited
you're listed with an @broadcom.com email address and there's the
copyright statement.

All of that said, I wasn't able to dig up a normative policy and either
of your proposed alternatives do exist in the kernel, so I withdraw any
objections regarding MODULE_AUTHOR. Take your pick.

Thierry

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

Re: [PATCH 2/5] pwm: kona: Introduce Kona PWM controller support

[Tim Kryger]

On Tue, Nov 26, 2013 at 1:45 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Nov 25, 2013 at 05:38:44PM -0800, Tim Kryger wrote:

> Okay, that's fine then. Do you have a pointer to that documentation?

Unfortunately the documentation is not publicly available at the moment.

>> The hardware is always enabled and at best can be be configured to
>> operate at zero duty.
>
> What good are the PWMOUT_ENABLE bits then? Is that really only used for
> triggering updates? That's what another comment suggests, but if so, can
> the comment in kona_pwmc_apply_settings() be extended to mention that?

The guidance from the documentation is to treat the enable bits as a trigger.

Sure I can expand the comment to try and make this more clear.

>> The settings in HW may have already been triggered and might not be
>> what you want.
>>
>> For example:
>>
>> /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable
>> /sys/class/pwm/pwmchip0/pwm1# echo 5000 > period
>> /sys/class/pwm/pwmchip0/pwm1# echo 2000 > duty_cycle
>> /sys/class/pwm/pwmchip0/pwm1# echo 0 > enable
>> /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable
>
> I'm not exactly sure what this is supposed to demonstrate.

The computation has to be performed for every operation except the disable.

> All of that said, I wasn't able to dig up a normative policy and either
> of your proposed alternatives do exist in the kernel, so I withdraw any
> objections regarding MODULE_AUTHOR. Take your pick.

Thanks