[PATCH v3 0/5] Add Broadcom Kona PWM Support

[PATCH v3 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.

Changes since v2:
  - SoC DTS file updated to use real clock's phandle + specifier
  - Toggle smooth mode off during apply so new settings take immediately

Changes since v1:
  - Fixed up macros to be clearer and more complete
  - Corrected spelling and punctuation mistakes
  - Added support for polarity
  - Made peripheral clock use more efficient
  - Made prescale and duty computation clearer
  - Moved Makefile addition to keep alphabetical
  - Split complex lines into multiple steps

Dependencies:
The "ARM: dts: Declare the PWM for bcm11351 (bcm281xx)" patch depends
upon "ARM: dts: bcm281xx: define real clocks" which is queued up in
for-next of arm-soc. See https://lkml.org/lkml/2014/2/14/451

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                         | 304 +++++++++++++++++++++
 7 files changed, 353 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 v3 1/5] Documentation: dt: Add Kona PWM binding

[Tim Kryger]

Add the binding description for the Kona PWM controller 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..c8e2d13
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
@@ -0,0 +1,24 @@
+Broadcom Kona PWM controller device tree bindings
+
+This controller has 6 channels.
+
+Required Properties :
+- compatible: should be "brcm,kona-pwm"
+- reg: physical base address and length of the controller's registers
+- clocks: phandle + clock specifier pair for the external clock
+- #pwm-cells: should be 3.  The first cell specifies the per-chip index
+  of the PWM to use, the second cell is the period in nanoseconds, and
+  the third cell is the flags.
+
+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 = <3>;
+};
-- 
1.8.0.1

Re: [PATCH v3 1/5] Documentation: dt: Add Kona PWM binding

[Thierry Reding]

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

On Wed, Mar 12, 2014 at 01:15:42PM -0700, Tim Kryger wrote:
> Add the binding description for the Kona PWM controller found on Broadcom's
> mobile SoCs.
> 
> Signed-off-by: Tim Kryger <tim.kryger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Reviewed-by: Alex Elder <elder-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Reviewed-by: Markus Mayer <markus.mayer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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..c8e2d13
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/bcm-kona-pwm.txt
> @@ -0,0 +1,24 @@
> +Broadcom Kona PWM controller device tree bindings
> +
> +This controller has 6 channels.
> +
> +Required Properties :
> +- compatible: should be "brcm,kona-pwm"

This is somewhat inconsistent because the example below clearly doesn't
set the compatible property to "brcm,kona-pwm". Perhaps better wording
would be:

	- compatible: should contain "brcm,kona-pwm"

> +- reg: physical base address and length of the controller's registers
> +- clocks: phandle + clock specifier pair for the external clock
> +- #pwm-cells: should be 3.  The first cell specifies the per-chip index
> +  of the PWM to use, the second cell is the period in nanoseconds, and
> +  the third cell is the flags.

Please use the canonical description for this:

	- #pwm-cells: Should be 3. See pwm.txt in this directory for a
	  description of the cells format.

> +Refer to pwm/pwm.txt for generic pwm controller node properties.

With the above, this line should go away. But still: "pwm controller" ->
"PWM controller".

Thierry

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

Re: [PATCH v3 1/5] Documentation: dt: Add Kona PWM binding

[Tim Kryger]

On Tue, Mar 18, 2014 at 2:18 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Mar 12, 2014 at 01:15:42PM -0700, Tim Kryger wrote:

>> @@ -0,0 +1,24 @@
>> +Broadcom Kona PWM controller device tree bindings
>> +
>> +This controller has 6 channels.
>> +
>> +Required Properties :
>> +- compatible: should be "brcm,kona-pwm"
>
> This is somewhat inconsistent because the example below clearly doesn't
> set the compatible property to "brcm,kona-pwm". Perhaps better wording
> would be:
>
>         - compatible: should contain "brcm,kona-pwm"

Agreed, what you wrote is more accurate.  I will update it.

>> +- reg: physical base address and length of the controller's registers
>> +- clocks: phandle + clock specifier pair for the external clock
>> +- #pwm-cells: should be 3.  The first cell specifies the per-chip index
>> +  of the PWM to use, the second cell is the period in nanoseconds, and
>> +  the third cell is the flags.
>
> Please use the canonical description for this:
>
>         - #pwm-cells: Should be 3. See pwm.txt in this directory for a
>           description of the cells format.
>
>> +Refer to pwm/pwm.txt for generic pwm controller node properties.
>
> With the above, this line should go away. But still: "pwm controller" ->
> "PWM controller".

Okay.  I will use the text you provided and drop the other line.

Sorry for missing the capitalization.  I thought I had fixed them all.

Thanks,
Tim

[PATCH v3 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Alex Elder <elder-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Markus Mayer <markus.mayer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/pwm/Kconfig        |  10 ++
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-bcm-kona.c | 304 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 315 insertions(+)
 create mode 100644 drivers/pwm/pwm-bcm-kona.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 22f2f28..1fd42af 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -62,6 +62,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 d8906ec..7413090 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)		+= pwm-atmel.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
+obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
new file mode 100644
index 0000000..374e9ad
--- /dev/null
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -0,0 +1,304 @@
+/*
+ * Copyright (C) 2014 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/delay.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 PWM_CONTROL_OFFSET			(0x00000000)
+#define PWM_CONTROL_SMOOTH_SHIFT(chan)		(24 + (chan))
+#define PWM_CONTROL_TYPE_SHIFT(chan)		(16 + (chan))
+#define PWM_CONTROL_POLARITY_SHIFT(chan)	(8 + (chan))
+#define PWM_CONTROL_ENABLE_SHIFT(chan)		(chan)
+
+#define PRESCALE_OFFSET				(0x00000004)
+#define PRESCALE_SHIFT(chan)			((chan) << 2)
+#define PRESCALE_MASK(chan)			(0x7 << PRESCALE_SHIFT(chan))
+#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, unsigned int chan)
+{
+	unsigned long value = readl(kp->base + PWM_CONTROL_OFFSET);
+
+	/*
+	 * New duty and period settings are only reflected in the PWM output
+	 * after a rising edge of the enable bit.  When smooth bit is set, the
+	 * new settings are delayed until the prior PWM period has completed.
+	 * Furthermore, if the smooth bit is set, the PWM continues to output a
+	 * waveform based on the old settings during the time that the enable
+	 * bit is low.  Otherwise the output is a constant high signal while
+	 * the enable bit is low.
+	 */
+
+	/* clear enable bit but set smooth bit to maintain old output */
+	value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+	value &= ~(1 << PWM_CONTROL_ENABLE_SHIFT(chan));
+	writel(value, kp->base + PWM_CONTROL_OFFSET);
+
+	/* set enable bit and clear smooth bit to apply new settings */
+	value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+	value |= (1 << PWM_CONTROL_ENABLE_SHIFT(chan));
+	writel(value, 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;
+	unsigned int value, 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);
+
+	/* There is polarity support in HW but it is easier to manage in SW */
+	if (pwm->polarity == PWM_POLARITY_INVERSED)
+		duty_ns = period_ns - duty_ns;
+
+	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 and dc are in bounds, the calculation is done */
+		if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
+			break;
+
+		/* Otherwise, increase prescale and recalculate pc and dc */
+		if (++prescale > PRESCALE_MAX)
+			return -EINVAL;
+	}
+
+	/* If the PWM channel is enabled, write the settings to the HW */
+	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+		value = readl(kp->base + PRESCALE_OFFSET);
+		value &= ~PRESCALE_MASK(chan);
+		value |= prescale << PRESCALE_SHIFT(chan);
+		writel(value, kp->base + PRESCALE_OFFSET);
+
+		writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
+
+		writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+
+		kona_pwmc_apply_settings(kp, chan);
+	}
+
+	return 0;
+}
+
+static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+				  enum pwm_polarity polarity)
+{
+	/*
+	 * The framework only allows the polarity to be changed when a PWM is
+	 * disabled so no immediate action is required here.  When a channel is
+	 * enabled, the polarity gets handled as part of the re-config step.
+	 */
+
+	return 0;
+}
+
+static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
+	int ret;
+
+	/*
+	 * The PWM framework does not clear the enable bit in the flags if an
+	 * error is returned from a PWM driver's enable function so it must be
+	 * cleared here if any trouble is encountered.
+	 */
+
+	ret = clk_prepare_enable(kp->clk);
+	if (ret < 0) {
+		dev_err(chip->dev, "failed to enable clock: %d\n", ret);
+		clear_bit(PWMF_ENABLED, &pwm->flags);
+		return ret;
+	}
+
+	ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
+	if (ret < 0) {
+		clk_disable_unprepare(kp->clk);
+		clear_bit(PWMF_ENABLED, &pwm->flags);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
+	unsigned int chan = pwm->hwpwm;
+
+	/*
+	 * The "enable" bits in the control register only affect when settings
+	 * start to take effect so the only real way to disable the PWM output
+	 * is to program a zero duty cycle.
+	 */
+
+	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
+	kona_pwmc_apply_settings(kp, chan);
+
+	/*
+	 * When the PWM clock is disabled, the output is pegged high or low
+	 * depending on its state at that instant.  To guarantee that the new
+	 * settings have taken effect and the output is low a delay of 400ns is
+	 * required.
+	 */
+
+	ndelay(400);
+
+	clk_disable_unprepare(kp->clk);
+}
+
+static const struct pwm_ops kona_pwm_ops = {
+	.config = kona_pwmc_config,
+	.set_polarity = kona_pwmc_set_polarity,
+	.enable = kona_pwmc_enable,
+	.disable = kona_pwmc_disable,
+	.owner = THIS_MODULE,
+};
+
+static int kona_pwmc_probe(struct platform_device *pdev)
+{
+	struct kona_pwmc *kp;
+	struct resource *res;
+	unsigned int chan, value;
+	int ret = 0;
+
+	kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL);
+	if (kp == NULL)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, kp);
+
+	kp->chip.dev = &pdev->dev;
+	kp->chip.ops = &kona_pwm_ops;
+	kp->chip.base = -1;
+	kp->chip.npwm = 6;
+	kp->chip.of_xlate = of_pwm_xlate_with_flags;
+	kp->chip.of_pwm_n_cells = 3;
+	kp->chip.can_sleep = true;
+
+	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, "failed to get clock: %ld\n",
+			PTR_ERR(kp->clk));
+		return PTR_ERR(kp->clk);
+	}
+
+	ret = clk_prepare_enable(kp->clk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to enable clock: %d\n", ret);
+		return ret;
+	}
+
+	/* Set smooth mode, push/pull, and normal polarity for all channels */
+	for (value = 0, chan = 0; chan < kp->chip.npwm; chan++) {
+		value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+		value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
+		value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
+	}
+	writel(value, kp->base + PWM_CONTROL_OFFSET);
+
+	clk_disable_unprepare(kp->clk);
+
+	ret = pwmchip_add(&kp->chip);
+	if (ret < 0)
+		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
+
+	return ret;
+}
+
+static int kona_pwmc_remove(struct platform_device *pdev)
+{
+	struct kona_pwmc *kp = platform_get_drvdata(pdev);
+	unsigned int chan;
+
+	for (chan = 0; chan < kp->chip.npwm; chan++)
+		if (test_bit(PWMF_ENABLED, &kp->chip.pwms[chan].flags))
+			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",
+		.of_match_table = bcm_kona_pwmc_dt,
+	},
+	.probe = kona_pwmc_probe,
+	.remove = kona_pwmc_remove,
+};
+
+module_platform_driver(kona_pwmc_driver);
+
+MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>");
+MODULE_AUTHOR("Tim Kryger <tkryger-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>");
+MODULE_DESCRIPTION("Driver for Kona PWM controller");
+MODULE_LICENSE("GPL v2");
-- 
1.8.0.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 7343 bytes --]

On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote:
[...]
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 22f2f28..1fd42af 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -62,6 +62,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

No "default y", please.

> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
[...]
> +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> +{
> +	unsigned long value = readl(kp->base + PWM_CONTROL_OFFSET);
> +
> +	/*
> +	 * New duty and period settings are only reflected in the PWM output
> +	 * after a rising edge of the enable bit.  When smooth bit is set, the
> +	 * new settings are delayed until the prior PWM period has completed.
> +	 * Furthermore, if the smooth bit is set, the PWM continues to output a
> +	 * waveform based on the old settings during the time that the enable
> +	 * bit is low.  Otherwise the output is a constant high signal while
> +	 * the enable bit is low.
> +	 */
> +
> +	/* clear enable bit but set smooth bit to maintain old output */
> +	value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));

There's no need for the parentheses here.

> +	value &= ~(1 << PWM_CONTROL_ENABLE_SHIFT(chan));
> +	writel(value, kp->base + PWM_CONTROL_OFFSET);
> +
> +	/* set enable bit and clear smooth bit to apply new settings */
> +	value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
> +	value |= (1 << PWM_CONTROL_ENABLE_SHIFT(chan));

Nor here.

> +	writel(value, kp->base + PWM_CONTROL_OFFSET);

I'm curious about how this work. The above writes the control register
once with smooth set and enable cleared, then immediately rewrites it
again with smooth cleared and enable set. Are these writes somehow
queued in hardware, so that the subsequent write becomes active only
after the current period?

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

Please align arguments on subsequent lines with those on the first line.

> +{
> +	struct kona_pwmc *kp = dev_get_drvdata(chip->dev);

The proper way to do this would be upcasting using container_of().
Better yet, define a static inline function that wraps container_of(),
just like very other driver does.

> +	u64 val, div, clk_rate;
> +	unsigned long prescale = PRESCALE_MIN, pc, dc;
> +	unsigned int value, 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);

"rate" is 50% shorter and would do equally well.

> +
> +	/* There is polarity support in HW but it is easier to manage in SW */
> +	if (pwm->polarity == PWM_POLARITY_INVERSED)
> +		duty_ns = period_ns - duty_ns;

No, this is wrong. You're not actually changing the *polarity* here.

Also I think this is missing a pair of clk_prepare_enable() and
clk_disable_unprepare().

> +static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +				  enum pwm_polarity polarity)
> +{
> +	/*
> +	 * The framework only allows the polarity to be changed when a PWM is
> +	 * disabled so no immediate action is required here.  When a channel is
> +	 * enabled, the polarity gets handled as part of the re-config step.
> +	 */
> +
> +	return 0;
> +}

See above. If you don't want to implement the hardware support for
inversed polarity, then simply don't implement this.

> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
> +	int ret;
> +
> +	/*
> +	 * The PWM framework does not clear the enable bit in the flags if an
> +	 * error is returned from a PWM driver's enable function so it must be
> +	 * cleared here if any trouble is encountered.
> +	 */
> +
> +	ret = clk_prepare_enable(kp->clk);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to enable clock: %d\n", ret);
> +		clear_bit(PWMF_ENABLED, &pwm->flags);

You're not supposed to touch these. This is a bug in the core, and it
should be fixed in the core.

> +		return ret;
> +	}
> +
> +	ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
> +	if (ret < 0) {
> +		clk_disable_unprepare(kp->clk);
> +		clear_bit(PWMF_ENABLED, &pwm->flags);
> +		return ret;
> +	}

Why are you doing this? .config() should be setting everything up
according to the given duty cycle and period and .enable() should only
be enabling a specific channel. Please don't conflate the two.

> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
> +	unsigned int chan = pwm->hwpwm;
> +
> +	/*
> +	 * The "enable" bits in the control register only affect when settings
> +	 * start to take effect so the only real way to disable the PWM output
> +	 * is to program a zero duty cycle.
> +	 */

What's wrong about waiting for the settings to take effect? There's
nothing about disable that says it should happen *immediately*.

> +
> +	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> +	kona_pwmc_apply_settings(kp, chan);
> +
> +	/*
> +	 * When the PWM clock is disabled, the output is pegged high or low
> +	 * depending on its state at that instant.  To guarantee that the new
> +	 * settings have taken effect and the output is low a delay of 400ns is
> +	 * required.
> +	 */
> +
> +	ndelay(400);

Where does this number come from?

> +static int kona_pwmc_probe(struct platform_device *pdev)
> +{
> +	struct kona_pwmc *kp;
> +	struct resource *res;
> +	unsigned int chan, value;
[...]
> +	/* Set smooth mode, push/pull, and normal polarity for all channels */
> +	for (value = 0, chan = 0; chan < kp->chip.npwm; chan++) {

Can't you initialize value to 0 when you declare it? That way the for
loop becomes much more idiomatic.

> +		value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
> +		value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
> +		value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
> +	}
> +	writel(value, kp->base + PWM_CONTROL_OFFSET);

I'd prefer a blank line between the above two for readability.

> +static struct platform_driver kona_pwmc_driver = {
> +

Gratuitous blank line.

> +	.driver = {
> +		.name = "bcm-kona-pwm",
> +		.of_match_table = bcm_kona_pwmc_dt,
> +	},
> +	.probe = kona_pwmc_probe,
> +	.remove = kona_pwmc_remove,
> +};
> +

And here.

> +module_platform_driver(kona_pwmc_driver);
> +
> +MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list@broadcom.com>");
> +MODULE_AUTHOR("Tim Kryger <tkryger@broadcom.com>");
> +MODULE_DESCRIPTION("Driver for Kona PWM controller");

Perhaps "Broadcom Kona PWM"?

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Tim Kryger]

On Tue, Mar 18, 2014 at 2:52 PM, Thierry Reding
<thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote:
> [...]
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 22f2f28..1fd42af 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -62,6 +62,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
>
> No "default y", please.

Even though it depends on ARCH_BCM_MOBILE?  It seems like a reasonable default.

These SoCs target mobile phones which almost always have a backlight
controlled by the PWM.

>
>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> [...]
>> +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>> +{
>> +     unsigned long value = readl(kp->base + PWM_CONTROL_OFFSET);
>> +
>> +     /*
>> +      * New duty and period settings are only reflected in the PWM output
>> +      * after a rising edge of the enable bit.  When smooth bit is set, the
>> +      * new settings are delayed until the prior PWM period has completed.
>> +      * Furthermore, if the smooth bit is set, the PWM continues to output a
>> +      * waveform based on the old settings during the time that the enable
>> +      * bit is low.  Otherwise the output is a constant high signal while
>> +      * the enable bit is low.
>> +      */
>> +
>> +     /* clear enable bit but set smooth bit to maintain old output */
>> +     value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>
> There's no need for the parentheses here.

Ok

>
>> +     value &= ~(1 << PWM_CONTROL_ENABLE_SHIFT(chan));
>> +     writel(value, kp->base + PWM_CONTROL_OFFSET);
>> +
>> +     /* set enable bit and clear smooth bit to apply new settings */
>> +     value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>> +     value |= (1 << PWM_CONTROL_ENABLE_SHIFT(chan));
>
> Nor here.

Ok

>
>> +     writel(value, kp->base + PWM_CONTROL_OFFSET);
>
> I'm curious about how this work. The above writes the control register
> once with smooth set and enable cleared, then immediately rewrites it
> again with smooth cleared and enable set. Are these writes somehow
> queued in hardware, so that the subsequent write becomes active only
> after the current period?

It isn't exactly queuing up the writes but if smooth mode is set, it
will wait to apply the new settings.

>
>> +static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> +                             int duty_ns, int period_ns)
>
> Please align arguments on subsequent lines with those on the first line.

Sure

>
>> +{
>> +     struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
>
> The proper way to do this would be upcasting using container_of().
> Better yet, define a static inline function that wraps container_of(),
> just like very other driver does.
>

I will convert to use this approach (except in kona_pwmc_remove)

>> +     u64 val, div, clk_rate;
>> +     unsigned long prescale = PRESCALE_MIN, pc, dc;
>> +     unsigned int value, 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);
>
> "rate" is 50% shorter and would do equally well.

That works for me.

>
>> +
>> +     /* There is polarity support in HW but it is easier to manage in SW */
>> +     if (pwm->polarity == PWM_POLARITY_INVERSED)
>> +             duty_ns = period_ns - duty_ns;
>
> No, this is wrong. You're not actually changing the *polarity* here.

Please elaborate.  I don't understand what is wrong here.

Does polarity influence the output while a PWM is disabled?

>
> Also I think this is missing a pair of clk_prepare_enable() and
> clk_disable_unprepare().

There is no issue here since the hardware registers are only touched
in kona_pwmc_config if the channel was already enabled.

>
>> +static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> +                               enum pwm_polarity polarity)
>> +{
>> +     /*
>> +      * The framework only allows the polarity to be changed when a PWM is
>> +      * disabled so no immediate action is required here.  When a channel is
>> +      * enabled, the polarity gets handled as part of the re-config step.
>> +      */
>> +
>> +     return 0;
>> +}
>
> See above. If you don't want to implement the hardware support for
> inversed polarity, then simply don't implement this.

I had originally planned to omit polarity support but because it
affects the binding (which is treated as ABI), it wouldn't be possible
to add it in later without defining a new compatible string.

>
>> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +     struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
>> +     int ret;
>> +
>> +     /*
>> +      * The PWM framework does not clear the enable bit in the flags if an
>> +      * error is returned from a PWM driver's enable function so it must be
>> +      * cleared here if any trouble is encountered.
>> +      */
>> +
>> +     ret = clk_prepare_enable(kp->clk);
>> +     if (ret < 0) {
>> +             dev_err(chip->dev, "failed to enable clock: %d\n", ret);
>> +             clear_bit(PWMF_ENABLED, &pwm->flags);
>
> You're not supposed to touch these. This is a bug in the core, and it
> should be fixed in the core.

Okay.  I'll drop the clear_bit lines from this patch.

>
>> +             return ret;
>> +     }
>> +
>> +     ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
>> +     if (ret < 0) {
>> +             clk_disable_unprepare(kp->clk);
>> +             clear_bit(PWMF_ENABLED, &pwm->flags);
>> +             return ret;
>> +     }
>
> Why are you doing this? .config() should be setting everything up
> according to the given duty cycle and period and .enable() should only
> be enabling a specific channel. Please don't conflate the two.

The hardware only supports a configure operation.

Thus disable has to be simulated by configuring zero duty.

During an enable operation we have to program the last configuration.

>
>> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +     struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
>> +     unsigned int chan = pwm->hwpwm;
>> +
>> +     /*
>> +      * The "enable" bits in the control register only affect when settings
>> +      * start to take effect so the only real way to disable the PWM output
>> +      * is to program a zero duty cycle.
>> +      */
>
> What's wrong about waiting for the settings to take effect? There's
> nothing about disable that says it should happen *immediately*.

The current code does wait for the settings to take effect.

Can you clarify what you mean?

>
>> +
>> +     writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
>> +     kona_pwmc_apply_settings(kp, chan);
>> +
>> +     /*
>> +      * When the PWM clock is disabled, the output is pegged high or low
>> +      * depending on its state at that instant.  To guarantee that the new
>> +      * settings have taken effect and the output is low a delay of 400ns is
>> +      * required.
>> +      */
>> +
>> +     ndelay(400);
>
> Where does this number come from?

Hardware documentation

>
>> +static int kona_pwmc_probe(struct platform_device *pdev)
>> +{
>> +     struct kona_pwmc *kp;
>> +     struct resource *res;
>> +     unsigned int chan, value;
> [...]
>> +     /* Set smooth mode, push/pull, and normal polarity for all channels */
>> +     for (value = 0, chan = 0; chan < kp->chip.npwm; chan++) {
>
> Can't you initialize value to 0 when you declare it? That way the for
> loop becomes much more idiomatic.

Sure.

>
>> +             value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>> +             value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
>> +             value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
>> +     }
>> +     writel(value, kp->base + PWM_CONTROL_OFFSET);
>
> I'd prefer a blank line between the above two for readability.

Ok

>
>> +static struct platform_driver kona_pwmc_driver = {
>> +
>
> Gratuitous blank line.

Ok

>
>> +     .driver = {
>> +             .name = "bcm-kona-pwm",
>> +             .of_match_table = bcm_kona_pwmc_dt,
>> +     },
>> +     .probe = kona_pwmc_probe,
>> +     .remove = kona_pwmc_remove,
>> +};
>> +
>
> And here.

Ok

>
>> +module_platform_driver(kona_pwmc_driver);
>> +
>> +MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>");
>> +MODULE_AUTHOR("Tim Kryger <tkryger-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>");
>> +MODULE_DESCRIPTION("Driver for Kona PWM controller");
>
> Perhaps "Broadcom Kona PWM"?

Sure

Thanks for the review.

-Tim
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tim Kryger]

On Tue, Mar 18, 2014 at 4:47 PM, Tim Kryger <tim.kryger@linaro.org> wrote:
> On Tue, Mar 18, 2014 at 2:52 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote:

>>> +
>>> +     /* There is polarity support in HW but it is easier to manage in SW */
>>> +     if (pwm->polarity == PWM_POLARITY_INVERSED)
>>> +             duty_ns = period_ns - duty_ns;
>>
>> No, this is wrong. You're not actually changing the *polarity* here.
>
> Please elaborate.  I don't understand what is wrong here.
>
> Does polarity influence the output while a PWM is disabled?
>

>>> +static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>>> +                               enum pwm_polarity polarity)
>>> +{
>>> +     /*
>>> +      * The framework only allows the polarity to be changed when a PWM is
>>> +      * disabled so no immediate action is required here.  When a channel is
>>> +      * enabled, the polarity gets handled as part of the re-config step.
>>> +      */
>>> +
>>> +     return 0;
>>> +}
>>
>> See above. If you don't want to implement the hardware support for
>> inversed polarity, then simply don't implement this.
>
> I had originally planned to omit polarity support but because it
> affects the binding (which is treated as ABI), it wouldn't be possible
> to add it in later without defining a new compatible string.

I would like to get this right but it occurred to me that there may be
a way to defer the implementation of this feature without disrupting
the binding.

Would it be acceptable to continue using #pwm-cells = <3> and
of_pwm_xlate_with_flags but return -EINVAL from kona_pwmc_set_polarity
if PWM_POLARITY_INVERSED is specified?

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

[Thierry Reding]

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

On Tue, Mar 18, 2014 at 06:06:03PM -0700, Tim Kryger wrote:
> On Tue, Mar 18, 2014 at 4:47 PM, Tim Kryger <tim.kryger@linaro.org> wrote:
> > On Tue, Mar 18, 2014 at 2:52 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> >> On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote:
[...]
> >>> +static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> >>> +                               enum pwm_polarity polarity)
> >>> +{
> >>> +     /*
> >>> +      * The framework only allows the polarity to be changed when a PWM is
> >>> +      * disabled so no immediate action is required here.  When a channel is
> >>> +      * enabled, the polarity gets handled as part of the re-config step.
> >>> +      */
> >>> +
> >>> +     return 0;
> >>> +}
> >>
> >> See above. If you don't want to implement the hardware support for
> >> inversed polarity, then simply don't implement this.
> >
> > I had originally planned to omit polarity support but because it
> > affects the binding (which is treated as ABI), it wouldn't be possible
> > to add it in later without defining a new compatible string.
> 
> I would like to get this right but it occurred to me that there may be
> a way to defer the implementation of this feature without disrupting
> the binding.
> 
> Would it be acceptable to continue using #pwm-cells = <3> and
> of_pwm_xlate_with_flags but return -EINVAL from kona_pwmc_set_polarity
> if PWM_POLARITY_INVERSED is specified?

This was recently discussed for the pwm-imx driver. And you can easily
support #pwm-cells = <2> and #pwm-cells = <3> with the same binding. So
you could start with #pwm-cells = <2>, leaving out .set_polarity() and
implement it later on, extending the binding in a backwards-compatible
way to support the polarity flag.

Thierry

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

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

[Thierry Reding]

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

On Tue, Mar 18, 2014 at 04:47:36PM -0700, Tim Kryger wrote:
> On Tue, Mar 18, 2014 at 2:52 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote:
> > [...]
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index 22f2f28..1fd42af 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -62,6 +62,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
> >
> > No "default y", please.
> 
> Even though it depends on ARCH_BCM_MOBILE?  It seems like a reasonable
> default.

Well, that's only true until somebody decides to turn that dependency
into something like:

	depends on ARCH_BCM_MOBILE || COMPILE_TEST

> These SoCs target mobile phones which almost always have a backlight
> controlled by the PWM.

And you can easily turn it on in bcm_defconfig and/or multi_v7_defconfig
if you want it enabled "by default".

> >> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
[...]
> >> +
> >> +     /* There is polarity support in HW but it is easier to manage in SW */
> >> +     if (pwm->polarity == PWM_POLARITY_INVERSED)
> >> +             duty_ns = period_ns - duty_ns;
> >
> > No, this is wrong. You're not actually changing the *polarity* here.
> 
> Please elaborate.  I don't understand what is wrong here.

Perhaps the comment for enum pwm_polarity in include/linux/pwm.h makes
this more obvious. What you do here is invert the duty cycle, rather
than the polarity. While it is true that the result is the same for
things like LEDs or backlight (because the signal power remains the
same), but there's a slight difference to what the PWM signal looks
like.

> Does polarity influence the output while a PWM is disabled?

Yes, there is apparently hardware where the polarity causes the PWM pin
to be 1 when the PWM is disabled. But that's really a separate issue.

> >> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >> +{
> >> +     struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
> >> +     int ret;
> >> +
> >> +     /*
> >> +      * The PWM framework does not clear the enable bit in the flags if an
> >> +      * error is returned from a PWM driver's enable function so it must be
> >> +      * cleared here if any trouble is encountered.
> >> +      */
> >> +
> >> +     ret = clk_prepare_enable(kp->clk);
> >> +     if (ret < 0) {
> >> +             dev_err(chip->dev, "failed to enable clock: %d\n", ret);
> >> +             clear_bit(PWMF_ENABLED, &pwm->flags);
> >
> > You're not supposed to touch these. This is a bug in the core, and it
> > should be fixed in the core.
> 
> Okay.  I'll drop the clear_bit lines from this patch.

I was sort of hoping you would post a patch which fixes this in the core
instead of just dropping those lines here and ignoring the issue. That
would've earned you bonus points. =)

> >> +             return ret;
> >> +     }
> >> +
> >> +     ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
> >> +     if (ret < 0) {
> >> +             clk_disable_unprepare(kp->clk);
> >> +             clear_bit(PWMF_ENABLED, &pwm->flags);
> >> +             return ret;
> >> +     }
> >
> > Why are you doing this? .config() should be setting everything up
> > according to the given duty cycle and period and .enable() should only
> > be enabling a specific channel. Please don't conflate the two.
> 
> The hardware only supports a configure operation.
> 
> Thus disable has to be simulated by configuring zero duty.
> 
> During an enable operation we have to program the last configuration.

My worry is that by calling one public function from another you'll be
mixing contexts. For instance, with the PWMF_ENABLED issue you mention
one solution to fix this in the core would be to delay setting the flag
until the driver's .enable() implementation actually succeeded. If such
an implementation was chosen, then your driver would be broken, because
PWMF_ENABLED wouldn't be set when .enable() is called, and therefore
calling the kona_pwmc_config() wouldn't have any effect.

> >> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >> +{
> >> +     struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
> >> +     unsigned int chan = pwm->hwpwm;
> >> +
> >> +     /*
> >> +      * The "enable" bits in the control register only affect when settings
> >> +      * start to take effect so the only real way to disable the PWM output
> >> +      * is to program a zero duty cycle.
> >> +      */
> >
> > What's wrong about waiting for the settings to take effect? There's
> > nothing about disable that says it should happen *immediately*.
> 
> The current code does wait for the settings to take effect.
> 
> Can you clarify what you mean?

Things are starting to get confusing here. Looking at the register
definitions, there's PWM_CONTROL_ENABLE_SHIFT(chan), which I suspect
controls whether or not a channel is enabled (if that's not the case
then please add a comment that explains what it does).

But a little further up you said that the hardware does only support a
configure operation and not an enable/disable.

The comment above further confuses me. What I read from it is that you
can in fact disable a channel by clearing the "enable" bit in the
control register. But the reason why you don't do it that way is because
that change won't take effect until "settings start to take effect". So
in order to disable the PWM immediately you resort to writing a 0 duty
cycle.

Perhaps I misunderstood, in which case it might be good to revise that
comment to be more explicit or accurate.

Thierry

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

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

[Tim Kryger]

On Thu, Mar 20, 2014 at 8:57 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, Mar 18, 2014 at 04:47:36PM -0700, Tim Kryger wrote:

> Perhaps the comment for enum pwm_polarity in include/linux/pwm.h makes
> this more obvious. What you do here is invert the duty cycle, rather
> than the polarity. While it is true that the result is the same for
> things like LEDs or backlight (because the signal power remains the
> same), but there's a slight difference to what the PWM signal looks
> like.

Thanks, I missed that comment before.

>> Does polarity influence the output while a PWM is disabled?
>
> Yes, there is apparently hardware where the polarity causes the PWM pin
> to be 1 when the PWM is disabled. But that's really a separate issue.

Do you have a preference on how this should be handled?


> Things are starting to get confusing here. Looking at the register
> definitions, there's PWM_CONTROL_ENABLE_SHIFT(chan), which I suspect
> controls whether or not a channel is enabled (if that's not the case
> then please add a comment that explains what it does).

I've tried to do this but the unfortunate name for these bits and
their nuanced behavior makes it difficult.

>
> But a little further up you said that the hardware does only support a
> configure operation and not an enable/disable.

If you define disabled as zero duty output, then this is true.

When the smooth bit is off and the "enable" bit is off output is 100% duty.

>
> The comment above further confuses me. What I read from it is that you
> can in fact disable a channel by clearing the "enable" bit in the
> control register. But the reason why you don't do it that way is because
> that change won't take effect until "settings start to take effect". So
> in order to disable the PWM immediately you resort to writing a 0 duty
> cycle.

Sorry if my comments were confusing.  New settings are only applied on
a rising edge of the "enable" bit.  You should think of it more as a
trigger bit than an enable.

>
> Perhaps I misunderstood, in which case it might be good to revise that
> comment to be more explicit or accurate.

Perhaps it would be clearest to deviate from the hw docs and rename
PWM_CONTROL_ENABLE_SHIFT to PWM_CONTROL_TRIGGER_SHIFT to more closely
match its function.  What do you think of the following?

/*
* New duty and period settings are only reflected in the PWM output
* after a rising edge of the trigger bit.  After a rising edge, if the
* smooth bit is set, the settings are also delayed until the current
* period has completed. Furthermore, if the smooth bit is set, the PWM
* continues to output a waveform based on the old settings during the
* time that the trigger bit is low.  Otherwise the output is a constant
* high signal while the trigger bit is low.
*/

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

[Tim Kryger]

On Thu, Mar 20, 2014 at 8:57 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, Mar 18, 2014 at 04:47:36PM -0700, Tim Kryger wrote:
>> On Tue, Mar 18, 2014 at 2:52 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote:

>> >> +     tristate "Kona PWM support"
>> >> +     depends on ARCH_BCM_MOBILE
>> >> +     default y
>> >
>> > No "default y", please.
>>
>> Even though it depends on ARCH_BCM_MOBILE?  It seems like a reasonable
>> default.
>
> Well, that's only true until somebody decides to turn that dependency
> into something like:
>
>         depends on ARCH_BCM_MOBILE || COMPILE_TEST

Do you expect someone would actually make such a change?

>
>> These SoCs target mobile phones which almost always have a backlight
>> controlled by the PWM.
>
> And you can easily turn it on in bcm_defconfig and/or multi_v7_defconfig
> if you want it enabled "by default".

Except that is not what default means.

Having a sensible default value in the Kconfig is convenient and
enables savedefconfig to create minimal configuration files.

Are you firmly opposed to this?

Thanks Again,
Tim

[PATCH v3 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Alex Elder <elder-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Markus Mayer <markus.mayer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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 64d069b..6b05ae6 100644
--- a/arch/arm/boot/dts/bcm11351.dtsi
+++ b/arch/arm/boot/dts/bcm11351.dtsi
@@ -193,6 +193,14 @@
 		status = "disabled";
 	};
 
+	pwm: pwm@3e01a000 {
+		compatible = "brcm,bcm11351-pwm", "brcm,kona-pwm";
+		reg = <0x3e01a000 0xcc>;
+		clocks = <&slave_ccu BCM281XX_SLAVE_CCU_PWM>;
+		#pwm-cells = <3>;
+		status = "disabled";
+	};
+
 	clocks {
 		#address-cells = <1>;
 		#size-cells = <1>;
-- 
1.8.0.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Alex Elder <elder-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Markus Mayer <markus.mayer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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 5ff2382..37c72eb 100644
--- a/arch/arm/boot/dts/bcm28155-ap.dts
+++ b/arch/arm/boot/dts/bcm28155-ap.dts
@@ -66,6 +66,10 @@
 		status = "okay";
 	};
 
+	pwm: pwm@3e01a000 {
+		status = "okay";
+	};
+
 	usbotg: usb@3f120000 {
 		status = "okay";
 	};
-- 
1.8.0.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tim Kryger]

Enable PWM drivers and the PWM-based backlight 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 2519d6d..b1898c3 100644
--- a/arch/arm/configs/bcm_defconfig
+++ b/arch/arm/configs/bcm_defconfig
@@ -84,6 +84,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
@@ -97,6 +98,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 v3 0/5] Add Broadcom Kona PWM Support

[Matt Porter]

On Wed, Mar 12, 2014 at 01:15:41PM -0700, Tim Kryger wrote:
> 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.
> 
> Changes since v2:
>   - SoC DTS file updated to use real clock's phandle + specifier
>   - Toggle smooth mode off during apply so new settings take immediately
> 
> Changes since v1:
>   - Fixed up macros to be clearer and more complete
>   - Corrected spelling and punctuation mistakes
>   - Added support for polarity
>   - Made peripheral clock use more efficient
>   - Made prescale and duty computation clearer
>   - Moved Makefile addition to keep alphabetical
>   - Split complex lines into multiple steps
> 
> Dependencies:
> The "ARM: dts: Declare the PWM for bcm11351 (bcm281xx)" patch depends
> upon "ARM: dts: bcm281xx: define real clocks" which is queued up in
> for-next of arm-soc. See https://lkml.org/lkml/2014/2/14/451
> 
> Tim Kryger (5):
>   Documentation: dt: Add Kona PWM binding
>   pwm: kona: Introduce Kona PWM controller support

Thierry, are you planning to merge these first two for 3.15? If so, I
will apply dts patches for my last mach-bcm 3.15 pull. I don't want to
do so until the binding is accepted.

-Matt

>   ARM: dts: Declare the PWM for bcm11351 (bcm281xx)
>   ARM: dts: Enable the PWM for bcm28155 AP board
>   ARM: bcm_defconfig: Enable PWM and Backlight

Re: [PATCH v3 0/5] Add Broadcom Kona PWM Support

[Thierry Reding]

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

On Fri, Mar 14, 2014 at 09:43:24AM -0400, Matt Porter wrote:
> On Wed, Mar 12, 2014 at 01:15:41PM -0700, Tim Kryger wrote:
> > 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.
> > 
> > Changes since v2:
> >   - SoC DTS file updated to use real clock's phandle + specifier
> >   - Toggle smooth mode off during apply so new settings take immediately
> > 
> > Changes since v1:
> >   - Fixed up macros to be clearer and more complete
> >   - Corrected spelling and punctuation mistakes
> >   - Added support for polarity
> >   - Made peripheral clock use more efficient
> >   - Made prescale and duty computation clearer
> >   - Moved Makefile addition to keep alphabetical
> >   - Split complex lines into multiple steps
> > 
> > Dependencies:
> > The "ARM: dts: Declare the PWM for bcm11351 (bcm281xx)" patch depends
> > upon "ARM: dts: bcm281xx: define real clocks" which is queued up in
> > for-next of arm-soc. See https://lkml.org/lkml/2014/2/14/451
> > 
> > Tim Kryger (5):
> >   Documentation: dt: Add Kona PWM binding
> >   pwm: kona: Introduce Kona PWM controller support
> 
> Thierry, are you planning to merge these first two for 3.15? If so, I
> will apply dts patches for my last mach-bcm 3.15 pull. I don't want to
> do so until the binding is accepted.

I've reviewed this and it doesn't look like 3.15 material.

Thierry

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