[PATCH 0/3] MIPS: JZ4740: Move PWM driver to PWM framework

[PATCH 0/3] MIPS: JZ4740: Move PWM driver to PWM framework

[Thierry Reding]

Hi,

This small series fixes a build error due to a circular header
dependency, exports the timer API so it can be used outside of
the arch/mips/jz4740 tree and finally moves and converts the
JZ4740 PWM driver to the PWM framework.

Note that I don't have any hardware to test this on, so I had to
rely on compile tests only. Patches 1 and 2 should probably go
through the MIPS tree, while I can take patch 3 through the PWM
tree. It touches a couple of files in arch/mips but the changes
are unlikely to cause conflicts.

Thierry

Thierry Reding (3):
  MIPS: JZ4740: Break circular header dependency
  MIPS: JZ4740: Export timer API
  pwm: Add Ingenic JZ4740 support

 arch/mips/include/asm/mach-jz4740/irq.h      |   5 +
 arch/mips/include/asm/mach-jz4740/platform.h |   1 +
 arch/mips/include/asm/mach-jz4740/timer.h    |  35 +++++
 arch/mips/jz4740/Kconfig                     |   3 -
 arch/mips/jz4740/Makefile                    |   2 +-
 arch/mips/jz4740/board-qi_lb60.c             |   3 +-
 arch/mips/jz4740/irq.h                       |  23 ---
 arch/mips/jz4740/platform.c                  |   6 +
 arch/mips/jz4740/pwm.c                       | 177 -----------------------
 arch/mips/jz4740/time.c                      |   2 +-
 arch/mips/jz4740/timer.c                     | 128 +++++++++++++++--
 arch/mips/jz4740/timer.h                     | 136 ------------------
 drivers/pwm/Kconfig                          |  12 +-
 drivers/pwm/Makefile                         |   1 +
 drivers/pwm/core.c                           |   2 +-
 drivers/pwm/pwm-jz4740.c                     | 205 +++++++++++++++++++++++++++
 16 files changed, 386 insertions(+), 355 deletions(-)
 delete mode 100644 arch/mips/jz4740/irq.h
 delete mode 100644 arch/mips/jz4740/pwm.c
 delete mode 100644 arch/mips/jz4740/timer.h
 create mode 100644 drivers/pwm/pwm-jz4740.c

-- 
1.7.12

[PATCH 1/3] MIPS: JZ4740: Break circular header dependency

[Thierry Reding]

When including irq.h, arch/mips/jz4740/irq.h will be selected as the
first candidate. This header does not include the proper definitions
(most notably NR_IRQS) required by subsequent headers. To solve this
arch/mips/jz4740/irq.h can be deleted and its contents can be moved
into arch/mips/include/asm/mach-jz4740/irq.h, which will then be
correctly included.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 arch/mips/include/asm/mach-jz4740/irq.h |  5 +++++
 arch/mips/jz4740/irq.h                  | 23 -----------------------
 2 files changed, 5 insertions(+), 23 deletions(-)
 delete mode 100644 arch/mips/jz4740/irq.h

diff --git a/arch/mips/include/asm/mach-jz4740/irq.h b/arch/mips/include/asm/mach-jz4740/irq.h
index 5ad1a9c..aa6fd90 100644
--- a/arch/mips/include/asm/mach-jz4740/irq.h
+++ b/arch/mips/include/asm/mach-jz4740/irq.h
@@ -54,4 +54,9 @@
 
 #define NR_IRQS (JZ4740_IRQ_ADC_BASE + 6)
 
+struct irq_data;
+
+extern void jz4740_irq_suspend(struct irq_data *data);
+extern void jz4740_irq_resume(struct irq_data *data);
+
 #endif
diff --git a/arch/mips/jz4740/irq.h b/arch/mips/jz4740/irq.h
deleted file mode 100644
index f75e39d..0000000
--- a/arch/mips/jz4740/irq.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/*
- *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
- *
- *  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;  either version 2 of the License, or (at your
- *  option) any later version.
- *
- *  You should have received a copy of the GNU General Public License along
- *  with this program; if not, write to the Free Software Foundation, Inc.,
- *  675 Mass Ave, Cambridge, MA 02139, USA.
- *
- */
-
-#ifndef __MIPS_JZ4740_IRQ_H__
-#define __MIPS_JZ4740_IRQ_H__
-
-#include <linux/irq.h>
-
-extern void jz4740_irq_suspend(struct irq_data *data);
-extern void jz4740_irq_resume(struct irq_data *data);
-
-#endif
-- 
1.7.12

Re: [PATCH 1/3] MIPS: JZ4740: Break circular header dependency

[Lars-Peter Clausen]

On 09/02/2012 11:52 AM, Thierry Reding wrote:
> When including irq.h, arch/mips/jz4740/irq.h will be selected as the
> first candidate. This header does not include the proper definitions
> (most notably NR_IRQS) required by subsequent headers. To solve this
> arch/mips/jz4740/irq.h can be deleted and its contents can be moved
> into arch/mips/include/asm/mach-jz4740/irq.h, which will then be
> correctly included.
> 

Where exactly did you have this problem? arch/mips/jz4740/ should not be in the
include path, so "#include <irq.h>" should not cause arch/mips/jz4740/irq.h to
be included.

> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
>  arch/mips/include/asm/mach-jz4740/irq.h |  5 +++++
>  arch/mips/jz4740/irq.h                  | 23 -----------------------
>  2 files changed, 5 insertions(+), 23 deletions(-)
>  delete mode 100644 arch/mips/jz4740/irq.h
> 
> diff --git a/arch/mips/include/asm/mach-jz4740/irq.h b/arch/mips/include/asm/mach-jz4740/irq.h
> index 5ad1a9c..aa6fd90 100644
> --- a/arch/mips/include/asm/mach-jz4740/irq.h
> +++ b/arch/mips/include/asm/mach-jz4740/irq.h
> @@ -54,4 +54,9 @@
>  
>  #define NR_IRQS (JZ4740_IRQ_ADC_BASE + 6)
>  
> +struct irq_data;
> +
> +extern void jz4740_irq_suspend(struct irq_data *data);
> +extern void jz4740_irq_resume(struct irq_data *data);
> +
>  #endif
> diff --git a/arch/mips/jz4740/irq.h b/arch/mips/jz4740/irq.h
> deleted file mode 100644
> index f75e39d..0000000
> --- a/arch/mips/jz4740/irq.h
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -/*
> - *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
> - *
> - *  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;  either version 2 of the License, or (at your
> - *  option) any later version.
> - *
> - *  You should have received a copy of the GNU General Public License along
> - *  with this program; if not, write to the Free Software Foundation, Inc.,
> - *  675 Mass Ave, Cambridge, MA 02139, USA.
> - *
> - */
> -
> -#ifndef __MIPS_JZ4740_IRQ_H__
> -#define __MIPS_JZ4740_IRQ_H__
> -
> -#include <linux/irq.h>
> -
> -extern void jz4740_irq_suspend(struct irq_data *data);
> -extern void jz4740_irq_resume(struct irq_data *data);
> -
> -#endif

Re: [PATCH 1/3] MIPS: JZ4740: Break circular header dependency

[Thierry Reding]

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

On Sun, Sep 02, 2012 at 04:48:46PM +0200, Lars-Peter Clausen wrote:
> On 09/02/2012 11:52 AM, Thierry Reding wrote:
> > When including irq.h, arch/mips/jz4740/irq.h will be selected as the
> > first candidate. This header does not include the proper definitions
> > (most notably NR_IRQS) required by subsequent headers. To solve this
> > arch/mips/jz4740/irq.h can be deleted and its contents can be moved
> > into arch/mips/include/asm/mach-jz4740/irq.h, which will then be
> > correctly included.
> > 
> 
> Where exactly did you have this problem? arch/mips/jz4740/ should not be in the
> include path, so "#include <irq.h>" should not cause arch/mips/jz4740/irq.h to
> be included.

While compile-testing I did make ARCH=mips menuconfig and selected
JZ4740 plus the new PWM driver option, then ran make ARCH=mips. That
showed errors like struct irq_data not being declared or NR_IRQS not
being defined. Maybe I was just using a very strange configuration.

I tested this with 3.6-rc3 and linux-next, both had this problem.
Neither of them had a default configuration for JZ4740, so maybe I'm
just missing a configuration option.

Thierry

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

[PATCH 2/3] MIPS: JZ4740: Export timer API

[Thierry Reding]

This is a prerequisite for allowing the PWM driver to be converted to
the PWM framework.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 arch/mips/include/asm/mach-jz4740/timer.h |  35 ++++++++
 arch/mips/jz4740/time.c                   |   2 +-
 arch/mips/jz4740/timer.c                  | 128 +++++++++++++++++++++++++---
 arch/mips/jz4740/timer.h                  | 136 ------------------------------
 4 files changed, 153 insertions(+), 148 deletions(-)
 delete mode 100644 arch/mips/jz4740/timer.h

diff --git a/arch/mips/include/asm/mach-jz4740/timer.h b/arch/mips/include/asm/mach-jz4740/timer.h
index 9baa03c..9e41d0e 100644
--- a/arch/mips/include/asm/mach-jz4740/timer.h
+++ b/arch/mips/include/asm/mach-jz4740/timer.h
@@ -16,6 +16,41 @@
 #ifndef __ASM_MACH_JZ4740_TIMER
 #define __ASM_MACH_JZ4740_TIMER
 
+#define JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN	BIT(9)
+#define JZ_TIMER_CTRL_PWM_ACTIVE_LOW		BIT(8)
+#define JZ_TIMER_CTRL_PWM_ENABLE		BIT(7)
+#define JZ_TIMER_CTRL_PRESCALE_MASK		0x1c
+#define JZ_TIMER_CTRL_PRESCALE_OFFSET		0x3
+#define JZ_TIMER_CTRL_PRESCALE_1		(0 << 3)
+#define JZ_TIMER_CTRL_PRESCALE_4		(1 << 3)
+#define JZ_TIMER_CTRL_PRESCALE_16		(2 << 3)
+#define JZ_TIMER_CTRL_PRESCALE_64		(3 << 3)
+#define JZ_TIMER_CTRL_PRESCALE_256		(4 << 3)
+#define JZ_TIMER_CTRL_PRESCALE_1024		(5 << 3)
+
+#define JZ_TIMER_CTRL_PRESCALER(x) ((x) << JZ_TIMER_CTRL_PRESCALE_OFFSET)
+
+#define JZ_TIMER_CTRL_SRC_EXT		BIT(2)
+#define JZ_TIMER_CTRL_SRC_RTC		BIT(1)
+#define JZ_TIMER_CTRL_SRC_PCLK		BIT(0)
+
+void __init jz4740_timer_init(void);
+
+void jz4740_timer_stop(unsigned int timer);
+void jz4740_timer_start(unsigned int timer);
+bool jz4740_timer_is_enabled(unsigned int timer);
+void jz4740_timer_enable(unsigned int timer);
+void jz4740_timer_disable(unsigned int timer);
+void jz4740_timer_set_period(unsigned int timer, uint16_t period);
+void jz4740_timer_set_duty(unsigned int timer, uint16_t duty);
+void jz4740_timer_set_count(unsigned int timer, uint16_t count);
+uint16_t jz4740_timer_get_count(unsigned int timer);
+void jz4740_timer_ack_full(unsigned int timer);
+void jz4740_timer_irq_full_enable(unsigned int timer);
+void jz4740_timer_irq_full_disable(unsigned int timer);
+uint16_t jz4740_timer_get_ctrl(unsigned int timer);
+void jz4740_timer_set_ctrl(unsigned int timer, uint16_t ctrl);
+
 void jz4740_timer_enable_watchdog(void);
 void jz4740_timer_disable_watchdog(void);
 
diff --git a/arch/mips/jz4740/time.c b/arch/mips/jz4740/time.c
index f83c2dd..39bb4bb 100644
--- a/arch/mips/jz4740/time.c
+++ b/arch/mips/jz4740/time.c
@@ -20,10 +20,10 @@
 #include <linux/clockchips.h>
 
 #include <asm/mach-jz4740/irq.h>
+#include <asm/mach-jz4740/timer.h>
 #include <asm/time.h>
 
 #include "clock.h"
-#include "timer.h"
 
 #define TIMER_CLOCKEVENT 0
 #define TIMER_CLOCKSOURCE 1
diff --git a/arch/mips/jz4740/timer.c b/arch/mips/jz4740/timer.c
index 654d5c3..79c4354 100644
--- a/arch/mips/jz4740/timer.c
+++ b/arch/mips/jz4740/timer.c
@@ -21,19 +21,28 @@
 
 #include <asm/mach-jz4740/base.h>
 
-void __iomem *jz4740_timer_base;
+#define JZ_REG_TIMER_STOP		0x0C
+#define JZ_REG_TIMER_STOP_SET		0x1C
+#define JZ_REG_TIMER_STOP_CLEAR		0x2C
+#define JZ_REG_TIMER_ENABLE		0x00
+#define JZ_REG_TIMER_ENABLE_SET		0x04
+#define JZ_REG_TIMER_ENABLE_CLEAR	0x08
+#define JZ_REG_TIMER_FLAG		0x10
+#define JZ_REG_TIMER_FLAG_SET		0x14
+#define JZ_REG_TIMER_FLAG_CLEAR		0x18
+#define JZ_REG_TIMER_MASK		0x20
+#define JZ_REG_TIMER_MASK_SET		0x24
+#define JZ_REG_TIMER_MASK_CLEAR		0x28
 
-void jz4740_timer_enable_watchdog(void)
-{
-	writel(BIT(16), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
-}
-EXPORT_SYMBOL_GPL(jz4740_timer_enable_watchdog);
+#define JZ_REG_TIMER_DFR(x) (((x) * 0x10) + 0x30)
+#define JZ_REG_TIMER_DHR(x) (((x) * 0x10) + 0x34)
+#define JZ_REG_TIMER_CNT(x) (((x) * 0x10) + 0x38)
+#define JZ_REG_TIMER_CTRL(x) (((x) * 0x10) + 0x3C)
 
-void jz4740_timer_disable_watchdog(void)
-{
-	writel(BIT(16), jz4740_timer_base + JZ_REG_TIMER_STOP_SET);
-}
-EXPORT_SYMBOL_GPL(jz4740_timer_disable_watchdog);
+#define JZ_TIMER_IRQ_HALF(x) BIT((x) + 0x10)
+#define JZ_TIMER_IRQ_FULL(x) BIT(x)
+
+void __iomem *jz4740_timer_base;
 
 void __init jz4740_timer_init(void)
 {
@@ -48,3 +57,100 @@ void __init jz4740_timer_init(void)
 	/* Timer irqs are unmasked by default, mask them */
 	writel(0x00ff00ff, jz4740_timer_base + JZ_REG_TIMER_MASK_SET);
 }
+
+void jz4740_timer_stop(unsigned int timer)
+{
+	writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_SET);
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_stop);
+
+void jz4740_timer_start(unsigned int timer)
+{
+	writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_start);
+
+bool jz4740_timer_is_enabled(unsigned int timer)
+{
+	return readb(jz4740_timer_base + JZ_REG_TIMER_ENABLE) & BIT(timer);
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_is_enabled);
+
+void jz4740_timer_enable(unsigned int timer)
+{
+	writeb(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_ENABLE_SET);
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_enable);
+
+void jz4740_timer_disable(unsigned int timer)
+{
+	writeb(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_ENABLE_CLEAR);
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_disable);
+
+void jz4740_timer_set_period(unsigned int timer, uint16_t period)
+{
+	writew(period, jz4740_timer_base + JZ_REG_TIMER_DFR(timer));
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_set_period);
+
+void jz4740_timer_set_duty(unsigned int timer, uint16_t duty)
+{
+	writew(duty, jz4740_timer_base + JZ_REG_TIMER_DHR(timer));
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_set_duty);
+
+void jz4740_timer_set_count(unsigned int timer, uint16_t count)
+{
+	writew(count, jz4740_timer_base + JZ_REG_TIMER_CNT(timer));
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_set_count);
+
+uint16_t jz4740_timer_get_count(unsigned int timer)
+{
+	return readw(jz4740_timer_base + JZ_REG_TIMER_CNT(timer));
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_get_count);
+
+void jz4740_timer_ack_full(unsigned int timer)
+{
+	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_FLAG_CLEAR);
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_ack_full);
+
+void jz4740_timer_irq_full_enable(unsigned int timer)
+{
+	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_FLAG_CLEAR);
+	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_MASK_CLEAR);
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_irq_full_enable);
+
+void jz4740_timer_irq_full_disable(unsigned int timer)
+{
+	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_MASK_SET);
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_irq_full_disable);
+
+void jz4740_timer_set_ctrl(unsigned int timer, uint16_t ctrl)
+{
+	writew(ctrl, jz4740_timer_base + JZ_REG_TIMER_CTRL(timer));
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_set_ctrl);
+
+uint16_t jz4740_timer_get_ctrl(unsigned int timer)
+{
+	return readw(jz4740_timer_base + JZ_REG_TIMER_CTRL(timer));
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_get_ctrl);
+
+void jz4740_timer_enable_watchdog(void)
+{
+	writel(BIT(16), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_enable_watchdog);
+
+void jz4740_timer_disable_watchdog(void)
+{
+	writel(BIT(16), jz4740_timer_base + JZ_REG_TIMER_STOP_SET);
+}
+EXPORT_SYMBOL_GPL(jz4740_timer_disable_watchdog);
diff --git a/arch/mips/jz4740/timer.h b/arch/mips/jz4740/timer.h
deleted file mode 100644
index fca3994..0000000
--- a/arch/mips/jz4740/timer.h
+++ /dev/null
@@ -1,136 +0,0 @@
-/*
- *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
- *  JZ4740 platform timer support
- *
- *  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;  either version 2 of the License, or (at your
- *  option) any later version.
- *
- *  You should have received a copy of the GNU General Public License along
- *  with this program; if not, write to the Free Software Foundation, Inc.,
- *  675 Mass Ave, Cambridge, MA 02139, USA.
- *
- */
-
-#ifndef __MIPS_JZ4740_TIMER_H__
-#define __MIPS_JZ4740_TIMER_H__
-
-#include <linux/module.h>
-#include <linux/io.h>
-
-#define JZ_REG_TIMER_STOP		0x0C
-#define JZ_REG_TIMER_STOP_SET		0x1C
-#define JZ_REG_TIMER_STOP_CLEAR		0x2C
-#define JZ_REG_TIMER_ENABLE		0x00
-#define JZ_REG_TIMER_ENABLE_SET		0x04
-#define JZ_REG_TIMER_ENABLE_CLEAR	0x08
-#define JZ_REG_TIMER_FLAG		0x10
-#define JZ_REG_TIMER_FLAG_SET		0x14
-#define JZ_REG_TIMER_FLAG_CLEAR		0x18
-#define JZ_REG_TIMER_MASK		0x20
-#define JZ_REG_TIMER_MASK_SET		0x24
-#define JZ_REG_TIMER_MASK_CLEAR		0x28
-
-#define JZ_REG_TIMER_DFR(x) (((x) * 0x10) + 0x30)
-#define JZ_REG_TIMER_DHR(x) (((x) * 0x10) + 0x34)
-#define JZ_REG_TIMER_CNT(x) (((x) * 0x10) + 0x38)
-#define JZ_REG_TIMER_CTRL(x) (((x) * 0x10) + 0x3C)
-
-#define JZ_TIMER_IRQ_HALF(x) BIT((x) + 0x10)
-#define JZ_TIMER_IRQ_FULL(x) BIT(x)
-
-#define JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN	BIT(9)
-#define JZ_TIMER_CTRL_PWM_ACTIVE_LOW		BIT(8)
-#define JZ_TIMER_CTRL_PWM_ENABLE		BIT(7)
-#define JZ_TIMER_CTRL_PRESCALE_MASK		0x1c
-#define JZ_TIMER_CTRL_PRESCALE_OFFSET		0x3
-#define JZ_TIMER_CTRL_PRESCALE_1		(0 << 3)
-#define JZ_TIMER_CTRL_PRESCALE_4		(1 << 3)
-#define JZ_TIMER_CTRL_PRESCALE_16		(2 << 3)
-#define JZ_TIMER_CTRL_PRESCALE_64		(3 << 3)
-#define JZ_TIMER_CTRL_PRESCALE_256		(4 << 3)
-#define JZ_TIMER_CTRL_PRESCALE_1024		(5 << 3)
-
-#define JZ_TIMER_CTRL_PRESCALER(x) ((x) << JZ_TIMER_CTRL_PRESCALE_OFFSET)
-
-#define JZ_TIMER_CTRL_SRC_EXT		BIT(2)
-#define JZ_TIMER_CTRL_SRC_RTC		BIT(1)
-#define JZ_TIMER_CTRL_SRC_PCLK		BIT(0)
-
-extern void __iomem *jz4740_timer_base;
-void __init jz4740_timer_init(void);
-
-static inline void jz4740_timer_stop(unsigned int timer)
-{
-	writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_SET);
-}
-
-static inline void jz4740_timer_start(unsigned int timer)
-{
-	writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
-}
-
-static inline bool jz4740_timer_is_enabled(unsigned int timer)
-{
-	return readb(jz4740_timer_base + JZ_REG_TIMER_ENABLE) & BIT(timer);
-}
-
-static inline void jz4740_timer_enable(unsigned int timer)
-{
-	writeb(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_ENABLE_SET);
-}
-
-static inline void jz4740_timer_disable(unsigned int timer)
-{
-	writeb(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_ENABLE_CLEAR);
-}
-
-
-static inline void jz4740_timer_set_period(unsigned int timer, uint16_t period)
-{
-	writew(period, jz4740_timer_base + JZ_REG_TIMER_DFR(timer));
-}
-
-static inline void jz4740_timer_set_duty(unsigned int timer, uint16_t duty)
-{
-	writew(duty, jz4740_timer_base + JZ_REG_TIMER_DHR(timer));
-}
-
-static inline void jz4740_timer_set_count(unsigned int timer, uint16_t count)
-{
-	writew(count, jz4740_timer_base + JZ_REG_TIMER_CNT(timer));
-}
-
-static inline uint16_t jz4740_timer_get_count(unsigned int timer)
-{
-	return readw(jz4740_timer_base + JZ_REG_TIMER_CNT(timer));
-}
-
-static inline void jz4740_timer_ack_full(unsigned int timer)
-{
-	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_FLAG_CLEAR);
-}
-
-static inline void jz4740_timer_irq_full_enable(unsigned int timer)
-{
-	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_FLAG_CLEAR);
-	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_MASK_CLEAR);
-}
-
-static inline void jz4740_timer_irq_full_disable(unsigned int timer)
-{
-	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_MASK_SET);
-}
-
-static inline void jz4740_timer_set_ctrl(unsigned int timer, uint16_t ctrl)
-{
-	writew(ctrl, jz4740_timer_base + JZ_REG_TIMER_CTRL(timer));
-}
-
-static inline uint16_t jz4740_timer_get_ctrl(unsigned int timer)
-{
-	return readw(jz4740_timer_base + JZ_REG_TIMER_CTRL(timer));
-}
-
-#endif
-- 
1.7.12

Re: [PATCH 2/3] MIPS: JZ4740: Export timer API

[Lars-Peter Clausen]

On 09/02/2012 11:52 AM, Thierry Reding wrote:
> This is a prerequisite for allowing the PWM driver to be converted to
> the PWM framework.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

I'd prefer to keep the timer functions inline, some of them are called quite
often in the system clock code.

> ---
>  arch/mips/include/asm/mach-jz4740/timer.h |  35 ++++++++
>  arch/mips/jz4740/time.c                   |   2 +-
>  arch/mips/jz4740/timer.c                  | 128 +++++++++++++++++++++++++---
>  arch/mips/jz4740/timer.h                  | 136 ------------------------------
>  4 files changed, 153 insertions(+), 148 deletions(-)
>  delete mode 100644 arch/mips/jz4740/timer.h
> 
> diff --git a/arch/mips/include/asm/mach-jz4740/timer.h b/arch/mips/include/asm/mach-jz4740/timer.h
> index 9baa03c..9e41d0e 100644
> --- a/arch/mips/include/asm/mach-jz4740/timer.h
> +++ b/arch/mips/include/asm/mach-jz4740/timer.h
> @@ -16,6 +16,41 @@
>  #ifndef __ASM_MACH_JZ4740_TIMER
>  #define __ASM_MACH_JZ4740_TIMER
>  
> +#define JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN	BIT(9)
> +#define JZ_TIMER_CTRL_PWM_ACTIVE_LOW		BIT(8)
> +#define JZ_TIMER_CTRL_PWM_ENABLE		BIT(7)
> +#define JZ_TIMER_CTRL_PRESCALE_MASK		0x1c
> +#define JZ_TIMER_CTRL_PRESCALE_OFFSET		0x3
> +#define JZ_TIMER_CTRL_PRESCALE_1		(0 << 3)
> +#define JZ_TIMER_CTRL_PRESCALE_4		(1 << 3)
> +#define JZ_TIMER_CTRL_PRESCALE_16		(2 << 3)
> +#define JZ_TIMER_CTRL_PRESCALE_64		(3 << 3)
> +#define JZ_TIMER_CTRL_PRESCALE_256		(4 << 3)
> +#define JZ_TIMER_CTRL_PRESCALE_1024		(5 << 3)
> +
> +#define JZ_TIMER_CTRL_PRESCALER(x) ((x) << JZ_TIMER_CTRL_PRESCALE_OFFSET)
> +
> +#define JZ_TIMER_CTRL_SRC_EXT		BIT(2)
> +#define JZ_TIMER_CTRL_SRC_RTC		BIT(1)
> +#define JZ_TIMER_CTRL_SRC_PCLK		BIT(0)
> +
> +void __init jz4740_timer_init(void);
> +
> +void jz4740_timer_stop(unsigned int timer);
> +void jz4740_timer_start(unsigned int timer);
> +bool jz4740_timer_is_enabled(unsigned int timer);
> +void jz4740_timer_enable(unsigned int timer);
> +void jz4740_timer_disable(unsigned int timer);
> +void jz4740_timer_set_period(unsigned int timer, uint16_t period);
> +void jz4740_timer_set_duty(unsigned int timer, uint16_t duty);
> +void jz4740_timer_set_count(unsigned int timer, uint16_t count);
> +uint16_t jz4740_timer_get_count(unsigned int timer);
> +void jz4740_timer_ack_full(unsigned int timer);
> +void jz4740_timer_irq_full_enable(unsigned int timer);
> +void jz4740_timer_irq_full_disable(unsigned int timer);
> +uint16_t jz4740_timer_get_ctrl(unsigned int timer);
> +void jz4740_timer_set_ctrl(unsigned int timer, uint16_t ctrl);
> +
>  void jz4740_timer_enable_watchdog(void);
>  void jz4740_timer_disable_watchdog(void);
>  
> diff --git a/arch/mips/jz4740/time.c b/arch/mips/jz4740/time.c
> index f83c2dd..39bb4bb 100644
> --- a/arch/mips/jz4740/time.c
> +++ b/arch/mips/jz4740/time.c
> @@ -20,10 +20,10 @@
>  #include <linux/clockchips.h>
>  
>  #include <asm/mach-jz4740/irq.h>
> +#include <asm/mach-jz4740/timer.h>
>  #include <asm/time.h>
>  
>  #include "clock.h"
> -#include "timer.h"
>  
>  #define TIMER_CLOCKEVENT 0
>  #define TIMER_CLOCKSOURCE 1
> diff --git a/arch/mips/jz4740/timer.c b/arch/mips/jz4740/timer.c
> index 654d5c3..79c4354 100644
> --- a/arch/mips/jz4740/timer.c
> +++ b/arch/mips/jz4740/timer.c
> @@ -21,19 +21,28 @@
>  
>  #include <asm/mach-jz4740/base.h>
>  
> -void __iomem *jz4740_timer_base;
> +#define JZ_REG_TIMER_STOP		0x0C
> +#define JZ_REG_TIMER_STOP_SET		0x1C
> +#define JZ_REG_TIMER_STOP_CLEAR		0x2C
> +#define JZ_REG_TIMER_ENABLE		0x00
> +#define JZ_REG_TIMER_ENABLE_SET		0x04
> +#define JZ_REG_TIMER_ENABLE_CLEAR	0x08
> +#define JZ_REG_TIMER_FLAG		0x10
> +#define JZ_REG_TIMER_FLAG_SET		0x14
> +#define JZ_REG_TIMER_FLAG_CLEAR		0x18
> +#define JZ_REG_TIMER_MASK		0x20
> +#define JZ_REG_TIMER_MASK_SET		0x24
> +#define JZ_REG_TIMER_MASK_CLEAR		0x28
>  
> -void jz4740_timer_enable_watchdog(void)
> -{
> -	writel(BIT(16), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
> -}
> -EXPORT_SYMBOL_GPL(jz4740_timer_enable_watchdog);
> +#define JZ_REG_TIMER_DFR(x) (((x) * 0x10) + 0x30)
> +#define JZ_REG_TIMER_DHR(x) (((x) * 0x10) + 0x34)
> +#define JZ_REG_TIMER_CNT(x) (((x) * 0x10) + 0x38)
> +#define JZ_REG_TIMER_CTRL(x) (((x) * 0x10) + 0x3C)
>  
> -void jz4740_timer_disable_watchdog(void)
> -{
> -	writel(BIT(16), jz4740_timer_base + JZ_REG_TIMER_STOP_SET);
> -}
> -EXPORT_SYMBOL_GPL(jz4740_timer_disable_watchdog);
> +#define JZ_TIMER_IRQ_HALF(x) BIT((x) + 0x10)
> +#define JZ_TIMER_IRQ_FULL(x) BIT(x)
> +
> +void __iomem *jz4740_timer_base;
>  
>  void __init jz4740_timer_init(void)
>  {
> @@ -48,3 +57,100 @@ void __init jz4740_timer_init(void)
>  	/* Timer irqs are unmasked by default, mask them */
>  	writel(0x00ff00ff, jz4740_timer_base + JZ_REG_TIMER_MASK_SET);
>  }
> +
> +void jz4740_timer_stop(unsigned int timer)
> +{
> +	writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_SET);
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_stop);
> +
> +void jz4740_timer_start(unsigned int timer)
> +{
> +	writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_start);
> +
> +bool jz4740_timer_is_enabled(unsigned int timer)
> +{
> +	return readb(jz4740_timer_base + JZ_REG_TIMER_ENABLE) & BIT(timer);
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_is_enabled);
> +
> +void jz4740_timer_enable(unsigned int timer)
> +{
> +	writeb(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_ENABLE_SET);
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_enable);
> +
> +void jz4740_timer_disable(unsigned int timer)
> +{
> +	writeb(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_ENABLE_CLEAR);
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_disable);
> +
> +void jz4740_timer_set_period(unsigned int timer, uint16_t period)
> +{
> +	writew(period, jz4740_timer_base + JZ_REG_TIMER_DFR(timer));
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_set_period);
> +
> +void jz4740_timer_set_duty(unsigned int timer, uint16_t duty)
> +{
> +	writew(duty, jz4740_timer_base + JZ_REG_TIMER_DHR(timer));
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_set_duty);
> +
> +void jz4740_timer_set_count(unsigned int timer, uint16_t count)
> +{
> +	writew(count, jz4740_timer_base + JZ_REG_TIMER_CNT(timer));
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_set_count);
> +
> +uint16_t jz4740_timer_get_count(unsigned int timer)
> +{
> +	return readw(jz4740_timer_base + JZ_REG_TIMER_CNT(timer));
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_get_count);
> +
> +void jz4740_timer_ack_full(unsigned int timer)
> +{
> +	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_FLAG_CLEAR);
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_ack_full);
> +
> +void jz4740_timer_irq_full_enable(unsigned int timer)
> +{
> +	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_FLAG_CLEAR);
> +	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_MASK_CLEAR);
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_irq_full_enable);
> +
> +void jz4740_timer_irq_full_disable(unsigned int timer)
> +{
> +	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_MASK_SET);
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_irq_full_disable);
> +
> +void jz4740_timer_set_ctrl(unsigned int timer, uint16_t ctrl)
> +{
> +	writew(ctrl, jz4740_timer_base + JZ_REG_TIMER_CTRL(timer));
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_set_ctrl);
> +
> +uint16_t jz4740_timer_get_ctrl(unsigned int timer)
> +{
> +	return readw(jz4740_timer_base + JZ_REG_TIMER_CTRL(timer));
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_get_ctrl);
> +
> +void jz4740_timer_enable_watchdog(void)
> +{
> +	writel(BIT(16), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_enable_watchdog);
> +
> +void jz4740_timer_disable_watchdog(void)
> +{
> +	writel(BIT(16), jz4740_timer_base + JZ_REG_TIMER_STOP_SET);
> +}
> +EXPORT_SYMBOL_GPL(jz4740_timer_disable_watchdog);
> diff --git a/arch/mips/jz4740/timer.h b/arch/mips/jz4740/timer.h
> deleted file mode 100644
> index fca3994..0000000
> --- a/arch/mips/jz4740/timer.h
> +++ /dev/null
> @@ -1,136 +0,0 @@
> -/*
> - *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
> - *  JZ4740 platform timer support
> - *
> - *  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;  either version 2 of the License, or (at your
> - *  option) any later version.
> - *
> - *  You should have received a copy of the GNU General Public License along
> - *  with this program; if not, write to the Free Software Foundation, Inc.,
> - *  675 Mass Ave, Cambridge, MA 02139, USA.
> - *
> - */
> -
> -#ifndef __MIPS_JZ4740_TIMER_H__
> -#define __MIPS_JZ4740_TIMER_H__
> -
> -#include <linux/module.h>
> -#include <linux/io.h>
> -
> -#define JZ_REG_TIMER_STOP		0x0C
> -#define JZ_REG_TIMER_STOP_SET		0x1C
> -#define JZ_REG_TIMER_STOP_CLEAR		0x2C
> -#define JZ_REG_TIMER_ENABLE		0x00
> -#define JZ_REG_TIMER_ENABLE_SET		0x04
> -#define JZ_REG_TIMER_ENABLE_CLEAR	0x08
> -#define JZ_REG_TIMER_FLAG		0x10
> -#define JZ_REG_TIMER_FLAG_SET		0x14
> -#define JZ_REG_TIMER_FLAG_CLEAR		0x18
> -#define JZ_REG_TIMER_MASK		0x20
> -#define JZ_REG_TIMER_MASK_SET		0x24
> -#define JZ_REG_TIMER_MASK_CLEAR		0x28
> -
> -#define JZ_REG_TIMER_DFR(x) (((x) * 0x10) + 0x30)
> -#define JZ_REG_TIMER_DHR(x) (((x) * 0x10) + 0x34)
> -#define JZ_REG_TIMER_CNT(x) (((x) * 0x10) + 0x38)
> -#define JZ_REG_TIMER_CTRL(x) (((x) * 0x10) + 0x3C)
> -
> -#define JZ_TIMER_IRQ_HALF(x) BIT((x) + 0x10)
> -#define JZ_TIMER_IRQ_FULL(x) BIT(x)
> -
> -#define JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN	BIT(9)
> -#define JZ_TIMER_CTRL_PWM_ACTIVE_LOW		BIT(8)
> -#define JZ_TIMER_CTRL_PWM_ENABLE		BIT(7)
> -#define JZ_TIMER_CTRL_PRESCALE_MASK		0x1c
> -#define JZ_TIMER_CTRL_PRESCALE_OFFSET		0x3
> -#define JZ_TIMER_CTRL_PRESCALE_1		(0 << 3)
> -#define JZ_TIMER_CTRL_PRESCALE_4		(1 << 3)
> -#define JZ_TIMER_CTRL_PRESCALE_16		(2 << 3)
> -#define JZ_TIMER_CTRL_PRESCALE_64		(3 << 3)
> -#define JZ_TIMER_CTRL_PRESCALE_256		(4 << 3)
> -#define JZ_TIMER_CTRL_PRESCALE_1024		(5 << 3)
> -
> -#define JZ_TIMER_CTRL_PRESCALER(x) ((x) << JZ_TIMER_CTRL_PRESCALE_OFFSET)
> -
> -#define JZ_TIMER_CTRL_SRC_EXT		BIT(2)
> -#define JZ_TIMER_CTRL_SRC_RTC		BIT(1)
> -#define JZ_TIMER_CTRL_SRC_PCLK		BIT(0)
> -
> -extern void __iomem *jz4740_timer_base;
> -void __init jz4740_timer_init(void);
> -
> -static inline void jz4740_timer_stop(unsigned int timer)
> -{
> -	writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_SET);
> -}
> -
> -static inline void jz4740_timer_start(unsigned int timer)
> -{
> -	writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR);
> -}
> -
> -static inline bool jz4740_timer_is_enabled(unsigned int timer)
> -{
> -	return readb(jz4740_timer_base + JZ_REG_TIMER_ENABLE) & BIT(timer);
> -}
> -
> -static inline void jz4740_timer_enable(unsigned int timer)
> -{
> -	writeb(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_ENABLE_SET);
> -}
> -
> -static inline void jz4740_timer_disable(unsigned int timer)
> -{
> -	writeb(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_ENABLE_CLEAR);
> -}
> -
> -
> -static inline void jz4740_timer_set_period(unsigned int timer, uint16_t period)
> -{
> -	writew(period, jz4740_timer_base + JZ_REG_TIMER_DFR(timer));
> -}
> -
> -static inline void jz4740_timer_set_duty(unsigned int timer, uint16_t duty)
> -{
> -	writew(duty, jz4740_timer_base + JZ_REG_TIMER_DHR(timer));
> -}
> -
> -static inline void jz4740_timer_set_count(unsigned int timer, uint16_t count)
> -{
> -	writew(count, jz4740_timer_base + JZ_REG_TIMER_CNT(timer));
> -}
> -
> -static inline uint16_t jz4740_timer_get_count(unsigned int timer)
> -{
> -	return readw(jz4740_timer_base + JZ_REG_TIMER_CNT(timer));
> -}
> -
> -static inline void jz4740_timer_ack_full(unsigned int timer)
> -{
> -	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_FLAG_CLEAR);
> -}
> -
> -static inline void jz4740_timer_irq_full_enable(unsigned int timer)
> -{
> -	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_FLAG_CLEAR);
> -	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_MASK_CLEAR);
> -}
> -
> -static inline void jz4740_timer_irq_full_disable(unsigned int timer)
> -{
> -	writel(JZ_TIMER_IRQ_FULL(timer), jz4740_timer_base + JZ_REG_TIMER_MASK_SET);
> -}
> -
> -static inline void jz4740_timer_set_ctrl(unsigned int timer, uint16_t ctrl)
> -{
> -	writew(ctrl, jz4740_timer_base + JZ_REG_TIMER_CTRL(timer));
> -}
> -
> -static inline uint16_t jz4740_timer_get_ctrl(unsigned int timer)
> -{
> -	return readw(jz4740_timer_base + JZ_REG_TIMER_CTRL(timer));
> -}
> -
> -#endif

Re: [PATCH 2/3] MIPS: JZ4740: Export timer API

[Thierry Reding]

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

On Sun, Sep 02, 2012 at 04:45:43PM +0200, Lars-Peter Clausen wrote:
> On 09/02/2012 11:52 AM, Thierry Reding wrote:
> > This is a prerequisite for allowing the PWM driver to be converted to
> > the PWM framework.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> 
> I'd prefer to keep the timer functions inline, some of them are called quite
> often in the system clock code.

I've opted for this variant because it better hides the register values.
If the functions are inlined it also means the complete register
definitions need to go into timer.h. If you don't think that's an issue,
I can update the patch accordingly.

Thierry

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

Re: [PATCH 2/3] MIPS: JZ4740: Export timer API

[Lars-Peter Clausen]

On 09/02/2012 10:21 PM, Thierry Reding wrote:
> On Sun, Sep 02, 2012 at 04:45:43PM +0200, Lars-Peter Clausen wrote:
>> On 09/02/2012 11:52 AM, Thierry Reding wrote:
>>> This is a prerequisite for allowing the PWM driver to be converted to
>>> the PWM framework.
>>>
>>> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
>>
>> I'd prefer to keep the timer functions inline, some of them are called quite
>> often in the system clock code.
> 
> I've opted for this variant because it better hides the register values.
> If the functions are inlined it also means the complete register
> definitions need to go into timer.h. If you don't think that's an issue,
> I can update the patch accordingly.
> 

It's not pretty, but it should be ok. Having a single global function for each
and every register access is kind of ugly too.

- Lars

Re: [PATCH 2/3] MIPS: JZ4740: Export timer API

[Thierry Reding]

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

On Sun, Sep 02, 2012 at 10:27:37PM +0200, Lars-Peter Clausen wrote:
> On 09/02/2012 10:21 PM, Thierry Reding wrote:
> > On Sun, Sep 02, 2012 at 04:45:43PM +0200, Lars-Peter Clausen wrote:
> >> On 09/02/2012 11:52 AM, Thierry Reding wrote:
> >>> This is a prerequisite for allowing the PWM driver to be converted to
> >>> the PWM framework.
> >>>
> >>> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> >>
> >> I'd prefer to keep the timer functions inline, some of them are called quite
> >> often in the system clock code.
> > 
> > I've opted for this variant because it better hides the register values.
> > If the functions are inlined it also means the complete register
> > definitions need to go into timer.h. If you don't think that's an issue,
> > I can update the patch accordingly.
> > 
> 
> It's not pretty, but it should be ok. Having a single global function for each
> and every register access is kind of ugly too.

Okay, I'll update the patch accordingly. I probably won't get around to
it until later this week because I won't have access to a computer for a
few days but I'll be back at work on September 10 and should be able to
send the next version of this series out then.

Thierry

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

[PATCH 3/3] pwm: Add Ingenic JZ4740 support

[Thierry Reding]

This commit moves the driver to drivers/pwm and converts it to the new
PWM framework.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 arch/mips/include/asm/mach-jz4740/platform.h |   1 +
 arch/mips/jz4740/Kconfig                     |   3 -
 arch/mips/jz4740/Makefile                    |   2 +-
 arch/mips/jz4740/board-qi_lb60.c             |   3 +-
 arch/mips/jz4740/platform.c                  |   6 +
 arch/mips/jz4740/pwm.c                       | 177 -----------------------
 drivers/pwm/Kconfig                          |  12 +-
 drivers/pwm/Makefile                         |   1 +
 drivers/pwm/core.c                           |   2 +-
 drivers/pwm/pwm-jz4740.c                     | 205 +++++++++++++++++++++++++++
 10 files changed, 228 insertions(+), 184 deletions(-)
 delete mode 100644 arch/mips/jz4740/pwm.c
 create mode 100644 drivers/pwm/pwm-jz4740.c

diff --git a/arch/mips/include/asm/mach-jz4740/platform.h b/arch/mips/include/asm/mach-jz4740/platform.h
index 564ab81..163e81d 100644
--- a/arch/mips/include/asm/mach-jz4740/platform.h
+++ b/arch/mips/include/asm/mach-jz4740/platform.h
@@ -31,6 +31,7 @@ extern struct platform_device jz4740_pcm_device;
 extern struct platform_device jz4740_codec_device;
 extern struct platform_device jz4740_adc_device;
 extern struct platform_device jz4740_wdt_device;
+extern struct platform_device jz4740_pwm_device;
 
 void jz4740_serial_device_register(void);
 
diff --git a/arch/mips/jz4740/Kconfig b/arch/mips/jz4740/Kconfig
index 3e7141f..4689030 100644
--- a/arch/mips/jz4740/Kconfig
+++ b/arch/mips/jz4740/Kconfig
@@ -7,6 +7,3 @@ config JZ4740_QI_LB60
 	bool "Qi Hardware Ben NanoNote"
 
 endchoice
-
-config HAVE_PWM
-	bool
diff --git a/arch/mips/jz4740/Makefile b/arch/mips/jz4740/Makefile
index e44abea..63bad0e 100644
--- a/arch/mips/jz4740/Makefile
+++ b/arch/mips/jz4740/Makefile
@@ -5,7 +5,7 @@
 # Object file lists.
 
 obj-y += prom.o irq.o time.o reset.o setup.o dma.o \
-	gpio.o clock.o platform.o timer.o pwm.o serial.o
+	gpio.o clock.o platform.o timer.o serial.o
 
 obj-$(CONFIG_DEBUG_FS) += clock-debugfs.o
 
diff --git a/arch/mips/jz4740/board-qi_lb60.c b/arch/mips/jz4740/board-qi_lb60.c
index 9a3d9de..405382c 100644
--- a/arch/mips/jz4740/board-qi_lb60.c
+++ b/arch/mips/jz4740/board-qi_lb60.c
@@ -394,7 +394,7 @@ static struct platform_device qi_lb60_pwm_beeper = {
 	.name = "pwm-beeper",
 	.id = -1,
 	.dev = {
-		.platform_data = (void *)4,
+		.platform_data = (void *)2,
 	},
 };
 
@@ -437,6 +437,7 @@ static struct platform_device *jz_platform_devices[] __initdata = {
 	&jz4740_codec_device,
 	&jz4740_rtc_device,
 	&jz4740_adc_device,
+	&jz4740_pwm_device,
 	&qi_lb60_gpio_keys,
 	&qi_lb60_pwm_beeper,
 	&qi_lb60_charger_device,
diff --git a/arch/mips/jz4740/platform.c b/arch/mips/jz4740/platform.c
index e342ed4..6d14dcd 100644
--- a/arch/mips/jz4740/platform.c
+++ b/arch/mips/jz4740/platform.c
@@ -323,3 +323,9 @@ struct platform_device jz4740_wdt_device = {
 	.num_resources = ARRAY_SIZE(jz4740_wdt_resources),
 	.resource      = jz4740_wdt_resources,
 };
+
+/* PWM */
+struct platform_device jz4740_pwm_device = {
+	.name = "jz4740-pwm",
+	.id   = -1,
+};
diff --git a/arch/mips/jz4740/pwm.c b/arch/mips/jz4740/pwm.c
deleted file mode 100644
index a26a6fa..0000000
--- a/arch/mips/jz4740/pwm.c
+++ /dev/null
@@ -1,177 +0,0 @@
-/*
- *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
- *  JZ4740 platform PWM support
- *
- *  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;  either version 2 of the License, or (at your
- *  option) any later version.
- *
- *  You should have received a copy of the GNU General Public License along
- *  with this program; if not, write to the Free Software Foundation, Inc.,
- *  675 Mass Ave, Cambridge, MA 02139, USA.
- *
- */
-
-#include <linux/kernel.h>
-
-#include <linux/clk.h>
-#include <linux/err.h>
-#include <linux/pwm.h>
-#include <linux/gpio.h>
-
-#include <asm/mach-jz4740/gpio.h>
-#include "timer.h"
-
-static struct clk *jz4740_pwm_clk;
-
-DEFINE_MUTEX(jz4740_pwm_mutex);
-
-struct pwm_device {
-	unsigned int id;
-	unsigned int gpio;
-	bool used;
-};
-
-static struct pwm_device jz4740_pwm_list[] = {
-	{ 2, JZ_GPIO_PWM2, false },
-	{ 3, JZ_GPIO_PWM3, false },
-	{ 4, JZ_GPIO_PWM4, false },
-	{ 5, JZ_GPIO_PWM5, false },
-	{ 6, JZ_GPIO_PWM6, false },
-	{ 7, JZ_GPIO_PWM7, false },
-};
-
-struct pwm_device *pwm_request(int id, const char *label)
-{
-	int ret = 0;
-	struct pwm_device *pwm;
-
-	if (id < 2 || id > 7 || !jz4740_pwm_clk)
-		return ERR_PTR(-ENODEV);
-
-	mutex_lock(&jz4740_pwm_mutex);
-
-	pwm = &jz4740_pwm_list[id - 2];
-	if (pwm->used)
-		ret = -EBUSY;
-	else
-		pwm->used = true;
-
-	mutex_unlock(&jz4740_pwm_mutex);
-
-	if (ret)
-		return ERR_PTR(ret);
-
-	ret = gpio_request(pwm->gpio, label);
-
-	if (ret) {
-		printk(KERN_ERR "Failed to request pwm gpio: %d\n", ret);
-		pwm->used = false;
-		return ERR_PTR(ret);
-	}
-
-	jz_gpio_set_function(pwm->gpio, JZ_GPIO_FUNC_PWM);
-
-	jz4740_timer_start(id);
-
-	return pwm;
-}
-
-void pwm_free(struct pwm_device *pwm)
-{
-	pwm_disable(pwm);
-	jz4740_timer_set_ctrl(pwm->id, 0);
-
-	jz_gpio_set_function(pwm->gpio, JZ_GPIO_FUNC_NONE);
-	gpio_free(pwm->gpio);
-
-	jz4740_timer_stop(pwm->id);
-
-	pwm->used = false;
-}
-
-int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
-{
-	unsigned long long tmp;
-	unsigned long period, duty;
-	unsigned int prescaler = 0;
-	unsigned int id = pwm->id;
-	uint16_t ctrl;
-	bool is_enabled;
-
-	if (duty_ns < 0 || duty_ns > period_ns)
-		return -EINVAL;
-
-	tmp = (unsigned long long)clk_get_rate(jz4740_pwm_clk) * period_ns;
-	do_div(tmp, 1000000000);
-	period = tmp;
-
-	while (period > 0xffff && prescaler < 6) {
-		period >>= 2;
-		++prescaler;
-	}
-
-	if (prescaler == 6)
-		return -EINVAL;
-
-	tmp = (unsigned long long)period * duty_ns;
-	do_div(tmp, period_ns);
-	duty = period - tmp;
-
-	if (duty >= period)
-		duty = period - 1;
-
-	is_enabled = jz4740_timer_is_enabled(id);
-	if (is_enabled)
-		pwm_disable(pwm);
-
-	jz4740_timer_set_count(id, 0);
-	jz4740_timer_set_duty(id, duty);
-	jz4740_timer_set_period(id, period);
-
-	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
-		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
-
-	jz4740_timer_set_ctrl(id, ctrl);
-
-	if (is_enabled)
-		pwm_enable(pwm);
-
-	return 0;
-}
-
-int pwm_enable(struct pwm_device *pwm)
-{
-	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->id);
-
-	ctrl |= JZ_TIMER_CTRL_PWM_ENABLE;
-	jz4740_timer_set_ctrl(pwm->id, ctrl);
-	jz4740_timer_enable(pwm->id);
-
-	return 0;
-}
-
-void pwm_disable(struct pwm_device *pwm)
-{
-	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->id);
-
-	ctrl &= ~JZ_TIMER_CTRL_PWM_ENABLE;
-	jz4740_timer_disable(pwm->id);
-	jz4740_timer_set_ctrl(pwm->id, ctrl);
-}
-
-static int __init jz4740_pwm_init(void)
-{
-	int ret = 0;
-
-	jz4740_pwm_clk = clk_get(NULL, "ext");
-
-	if (IS_ERR(jz4740_pwm_clk)) {
-		ret = PTR_ERR(jz4740_pwm_clk);
-		jz4740_pwm_clk = NULL;
-	}
-
-	return ret;
-}
-subsys_initcall(jz4740_pwm_init);
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 90c5c73..5c663df 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -1,6 +1,6 @@
 menuconfig PWM
 	bool "Pulse-Width Modulation (PWM) Support"
-	depends on !MACH_JZ4740 && !PUV3_PWM
+	depends on !PUV3_PWM
 	help
 	  Generic Pulse-Width Modulation (PWM) support.
 
@@ -47,6 +47,16 @@ config PWM_IMX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-imx.
 
+config PWM_JZ4740
+	tristate "Ingenic JZ4740 PWM support"
+	depends on MACH_JZ4740
+	help
+	  Generic PWM framework driver for Ingenic JZ4740 based
+	  machines.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-jz4740.
+
 config PWM_LPC32XX
 	tristate "LPC32XX PWM support"
 	depends on ARCH_LPC32XX
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index e4b2c89..a1d6169 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1,6 +1,7 @@
 obj-$(CONFIG_PWM)		+= core.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
+obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 92b1782..f5acdaa 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -371,7 +371,7 @@ EXPORT_SYMBOL_GPL(pwm_free);
  */
 int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
 {
-	if (!pwm || period_ns == 0 || duty_ns > period_ns)
+	if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
 		return -EINVAL;
 
 	return pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
new file mode 100644
index 0000000..db29b37
--- /dev/null
+++ b/drivers/pwm/pwm-jz4740.c
@@ -0,0 +1,205 @@
+/*
+ *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
+ *  JZ4740 platform PWM support
+ *
+ *  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;  either version 2 of the License, or (at your
+ *  option) any later version.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+#include <asm/mach-jz4740/gpio.h>
+#include <timer.h>
+
+#define NUM_PWM 6
+
+static const unsigned int jz4740_pwm_gpio_list[NUM_PWM] = {
+	JZ_GPIO_PWM2,
+	JZ_GPIO_PWM3,
+	JZ_GPIO_PWM4,
+	JZ_GPIO_PWM5,
+	JZ_GPIO_PWM6,
+	JZ_GPIO_PWM7,
+};
+
+struct jz4740_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+};
+
+static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
+{
+	return container_of(chip, struct jz4740_pwm_chip, chip);
+}
+
+static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	unsigned int gpio = jz4740_pwm_gpio_list[pwm->hwpwm];
+	int ret;
+
+	ret = gpio_request(gpio, pwm->label);
+
+	if (ret) {
+		printk(KERN_ERR "Failed to request pwm gpio: %d\n", ret);
+		return ret;
+	}
+
+	jz_gpio_set_function(gpio, JZ_GPIO_FUNC_PWM);
+
+	jz4740_timer_start(pwm->hwpwm);
+
+	return 0;
+}
+
+static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	unsigned int gpio = jz4740_pwm_gpio_list[pwm->hwpwm];
+
+	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
+
+	jz_gpio_set_function(gpio, JZ_GPIO_FUNC_NONE);
+	gpio_free(gpio);
+
+	jz4740_timer_stop(pwm->hwpwm);
+}
+
+static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			     int duty_ns, int period_ns)
+{
+	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
+	unsigned long long tmp;
+	unsigned long period, duty;
+	unsigned int prescaler = 0;
+	uint16_t ctrl;
+	bool is_enabled;
+
+	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
+	do_div(tmp, 1000000000);
+	period = tmp;
+
+	while (period > 0xffff && prescaler < 6) {
+		period >>= 2;
+		++prescaler;
+	}
+
+	if (prescaler == 6)
+		return -EINVAL;
+
+	tmp = (unsigned long long)period * duty_ns;
+	do_div(tmp, period_ns);
+	duty = period - tmp;
+
+	if (duty >= period)
+		duty = period - 1;
+
+	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
+	if (is_enabled)
+		pwm_disable(pwm);
+
+	jz4740_timer_set_count(pwm->hwpwm, 0);
+	jz4740_timer_set_duty(pwm->hwpwm, duty);
+	jz4740_timer_set_period(pwm->hwpwm, period);
+
+	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
+		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
+
+	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+
+	if (is_enabled)
+		pwm_enable(pwm);
+
+	return 0;
+}
+
+static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
+
+	ctrl |= JZ_TIMER_CTRL_PWM_ENABLE;
+	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+	jz4740_timer_enable(pwm->hwpwm);
+
+	return 0;
+}
+
+static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
+
+	ctrl &= ~JZ_TIMER_CTRL_PWM_ENABLE;
+	jz4740_timer_disable(pwm->hwpwm);
+	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+}
+
+static const struct pwm_ops jz4740_pwm_ops = {
+	.request = jz4740_pwm_request,
+	.free = jz4740_pwm_free,
+	.config = jz4740_pwm_config,
+	.enable = jz4740_pwm_enable,
+	.disable = jz4740_pwm_disable,
+};
+
+static int jz4740_pwm_probe(struct platform_device *pdev)
+{
+	struct jz4740_pwm_chip *jz4740;
+	int ret = 0;
+
+	jz4740 = devm_kzalloc(&pdev->dev, sizeof(*jz4740), GFP_KERNEL);
+	if (!jz4740)
+		return -ENOMEM;
+
+	jz4740->clk = clk_get(NULL, "ext");
+	if (IS_ERR(jz4740->clk))
+		return PTR_ERR(jz4740->clk);
+
+	jz4740->chip.dev = &pdev->dev;
+	jz4740->chip.ops = &jz4740_pwm_ops;
+	jz4740->chip.npwm = NUM_PWM;
+	jz4740->chip.base = -1;
+
+	ret = pwmchip_add(&jz4740->chip);
+	if (ret < 0) {
+		clk_put(jz4740->clk);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, jz4740);
+
+	return 0;
+}
+
+static int jz4740_pwm_remove(struct platform_device *pdev)
+{
+	struct jz4740_pwm_chip *jz4740 = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&jz4740->chip);
+	if (ret < 0)
+		return ret;
+
+	clk_put(jz4740->clk);
+
+	return 0;
+}
+
+static struct platform_driver jz4740_pwm_driver = {
+	.driver = {
+		.name = "jz4740-pwm",
+	},
+	.probe = jz4740_pwm_probe,
+	.remove = jz4740_pwm_remove,
+};
+module_platform_driver(jz4740_pwm_driver);
-- 
1.7.12

Re: [PATCH 3/3] pwm: Add Ingenic JZ4740 support

[Lars-Peter Clausen]

On 09/02/2012 11:52 AM, Thierry Reding wrote:
> This commit moves the driver to drivers/pwm and converts it to the new
> PWM framework.
> 

Thanks for taking care of this, some comments inline.

[...]
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 92b1782..f5acdaa 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -371,7 +371,7 @@ EXPORT_SYMBOL_GPL(pwm_free);
>   */
>  int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
> -	if (!pwm || period_ns == 0 || duty_ns > period_ns)
> +	if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
>  		return -EINVAL;
>  

This change seems to be unrelated.

>  	return pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> new file mode 100644
> index 0000000..db29b37
> --- /dev/null
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -0,0 +1,205 @@
> +/*
> + *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
> + *  JZ4740 platform PWM support
> + *
> + *  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;  either version 2 of the License, or (at your
> + *  option) any later version.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  675 Mass Ave, Cambridge, MA 02139, USA.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +#include <asm/mach-jz4740/gpio.h>
> +#include <timer.h>

#include <asm/mach-jz4740/timer.h>

> +
> +#define NUM_PWM 6
> +
> +static const unsigned int jz4740_pwm_gpio_list[NUM_PWM] = {

As mth said, it would be better to have JZ_GPIO_PWM0 and here as well and set
NUM_PWM to 8. Right now we are using the timers associated to PWM channel 0 and
1 as system timers. But there might be devices where this is not possible, e.g.
because the PWM is actually connected to something. Also this fixes the of by
two for the hwpwm id.

> +	JZ_GPIO_PWM2,
> +	JZ_GPIO_PWM3,
> +	JZ_GPIO_PWM4,
> +	JZ_GPIO_PWM5,
> +	JZ_GPIO_PWM6,
> +	JZ_GPIO_PWM7,
> +};
> +
> +struct jz4740_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +};
> +
> +static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct jz4740_pwm_chip, chip);
> +}
> +
> +static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	unsigned int gpio = jz4740_pwm_gpio_list[pwm->hwpwm];
> +	int ret;
> +
> +	ret = gpio_request(gpio, pwm->label);
> +
> +	if (ret) {
> +		printk(KERN_ERR "Failed to request pwm gpio: %d\n", ret);

dev_err(chip->dev, ....

> +		return ret;
> +	}
> +
> +	jz_gpio_set_function(gpio, JZ_GPIO_FUNC_PWM);
> +
> +	jz4740_timer_start(pwm->hwpwm);
> +
> +	return 0;
> +}
> +
> +static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	unsigned int gpio = jz4740_pwm_gpio_list[pwm->hwpwm];
> +
> +	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
> +
> +	jz_gpio_set_function(gpio, JZ_GPIO_FUNC_NONE);
> +	gpio_free(gpio);
> +
> +	jz4740_timer_stop(pwm->hwpwm);
> +}
> +
> +static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     int duty_ns, int period_ns)
> +{
> +	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
> +	unsigned long long tmp;
> +	unsigned long period, duty;
> +	unsigned int prescaler = 0;
> +	uint16_t ctrl;
> +	bool is_enabled;
> +
> +	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
> +	do_div(tmp, 1000000000);
> +	period = tmp;
> +
> +	while (period > 0xffff && prescaler < 6) {
> +		period >>= 2;
> +		++prescaler;
> +	}
> +
> +	if (prescaler == 6)
> +		return -EINVAL;
> +
> +	tmp = (unsigned long long)period * duty_ns;
> +	do_div(tmp, period_ns);
> +	duty = period - tmp;
> +
> +	if (duty >= period)
> +		duty = period - 1;
> +
> +	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
> +	if (is_enabled)
> +		pwm_disable(pwm);

I think this should be jz4740_pwm_disable

> +
> +	jz4740_timer_set_count(pwm->hwpwm, 0);
> +	jz4740_timer_set_duty(pwm->hwpwm, duty);
> +	jz4740_timer_set_period(pwm->hwpwm, period);
> +
> +	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
> +		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
> +
> +	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
> +
> +	if (is_enabled)
> +		pwm_enable(pwm);

and jz4740_pwm_enable here.

> +
> +	return 0;
> +}
> +

> +
> +static const struct pwm_ops jz4740_pwm_ops = {
> +	.request = jz4740_pwm_request,
> +	.free = jz4740_pwm_free,
> +	.config = jz4740_pwm_config,
> +	.enable = jz4740_pwm_enable,
> +	.disable = jz4740_pwm_disable,

.owner = THIS_MODULE,

> +};
> +
> +static int jz4740_pwm_probe(struct platform_device *pdev)
__devinit
> +{
> +	struct jz4740_pwm_chip *jz4740;
> +	int ret = 0;

The '= 0' is not really necessary since it will be overwritten anyway.

> +
> +	jz4740 = devm_kzalloc(&pdev->dev, sizeof(*jz4740), GFP_KERNEL);
> +	if (!jz4740)
> +		return -ENOMEM;
> +
> +	jz4740->clk = clk_get(NULL, "ext");
> +	if (IS_ERR(jz4740->clk))
> +		return PTR_ERR(jz4740->clk);
> +
> +	jz4740->chip.dev = &pdev->dev;
> +	jz4740->chip.ops = &jz4740_pwm_ops;
> +	jz4740->chip.npwm = NUM_PWM;
> +	jz4740->chip.base = -1;
> +
> +	ret = pwmchip_add(&jz4740->chip);
> +	if (ret < 0) {
> +		clk_put(jz4740->clk);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, jz4740);
> +
> +	return 0;
> +}
> +
> +static int jz4740_pwm_remove(struct platform_device *pdev)
__devexit
> +{
> +	struct jz4740_pwm_chip *jz4740 = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = pwmchip_remove(&jz4740->chip);
> +	if (ret < 0)
> +		return ret;

remove is not really allowed to fail, the return value is never really tested
and the device is removed nevertheless. But this seems to be a problem with the
PWM API. It should be possible to remove a PWM chip even if it is currently in
use and after a PWM chip has been removed all calls to a pwm_device of that
chip it should return an error. This will require reference counting for the
pwm_device struct though. E.g. by adding a 'struct device' to it.

> +
> +	clk_put(jz4740->clk);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver jz4740_pwm_driver = {
> +	.driver = {
> +		.name = "jz4740-pwm",

.owner = THIS_MODULE,

> +	},
> +	.probe = jz4740_pwm_probe,
> +	.remove = jz4740_pwm_remove,

.remove = __devexit_p(jz4740_pwm_remove),

> +};
> +module_platform_driver(jz4740_pwm_driver);

MODULE_LICENSE(...), MODULE_AUTHOR(...), MODULE_DESCRIPTION(...), MODULE_ALIAS(...)

Re: [PATCH 3/3] pwm: Add Ingenic JZ4740 support

[Thierry Reding]

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

On Sun, Sep 02, 2012 at 04:44:15PM +0200, Lars-Peter Clausen wrote:
> On 09/02/2012 11:52 AM, Thierry Reding wrote:
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 92b1782..f5acdaa 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -371,7 +371,7 @@ EXPORT_SYMBOL_GPL(pwm_free);
> >   */
> >  int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> > -	if (!pwm || period_ns == 0 || duty_ns > period_ns)
> > +	if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
> >  		return -EINVAL;
> >  
> 
> This change seems to be unrelated.

Yes, that slipped in by mistake. That was supposed to go into a separate
patch so that the .config of each driver doesn't have to repeat these
checks.

> >  	return pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> > diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> > new file mode 100644
> > index 0000000..db29b37
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-jz4740.c
> > @@ -0,0 +1,205 @@
> > +/*
> > + *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
> > + *  JZ4740 platform PWM support
> > + *
> > + *  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;  either version 2 of the License, or (at your
> > + *  option) any later version.
> > + *
> > + *  You should have received a copy of the GNU General Public License along
> > + *  with this program; if not, write to the Free Software Foundation, Inc.,
> > + *  675 Mass Ave, Cambridge, MA 02139, USA.
> > + *
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +
> > +#include <asm/mach-jz4740/gpio.h>
> > +#include <timer.h>
> 
> #include <asm/mach-jz4740/timer.h>
> 
> > +
> > +#define NUM_PWM 6
> > +
> > +static const unsigned int jz4740_pwm_gpio_list[NUM_PWM] = {
> 
> As mth said, it would be better to have JZ_GPIO_PWM0 and here as well and set
> NUM_PWM to 8. Right now we are using the timers associated to PWM channel 0 and
> 1 as system timers. But there might be devices where this is not possible, e.g.
> because the PWM is actually connected to something. Also this fixes the of by
> two for the hwpwm id.

Okay. I was actually planning on doing some cleanup in a follow-up patch
and try to limit actual changes in this patch to what is required by the
conversion. However if Maarten and you both are okay with it I can make
these additional changes while at it.

> > +	if (ret) {
> > +		printk(KERN_ERR "Failed to request pwm gpio: %d\n", ret);
> 
> dev_err(chip->dev, ....

Okay.

> > +	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
> > +	if (is_enabled)
> > +		pwm_disable(pwm);
> 
> I think this should be jz4740_pwm_disable
> 
> > +
> > +	jz4740_timer_set_count(pwm->hwpwm, 0);
> > +	jz4740_timer_set_duty(pwm->hwpwm, duty);
> > +	jz4740_timer_set_period(pwm->hwpwm, period);
> > +
> > +	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
> > +		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
> > +
> > +	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
> > +
> > +	if (is_enabled)
> > +		pwm_enable(pwm);
> 
> and jz4740_pwm_enable here.

I wonder if this is actually required here. Can the timer really not be
reprogrammed while enabled?

> > +
> > +	return 0;
> > +}
> > +
> 
> > +
> > +static const struct pwm_ops jz4740_pwm_ops = {
> > +	.request = jz4740_pwm_request,
> > +	.free = jz4740_pwm_free,
> > +	.config = jz4740_pwm_config,
> > +	.enable = jz4740_pwm_enable,
> > +	.disable = jz4740_pwm_disable,
> 
> .owner = THIS_MODULE,

Yes, I forgot that one.

> > +};
> > +
> > +static int jz4740_pwm_probe(struct platform_device *pdev)
> __devinit

Yes, I'll add that.

> > +{
> > +	struct jz4740_pwm_chip *jz4740;
> > +	int ret = 0;
> 
> The '= 0' is not really necessary since it will be overwritten anyway.

Right, I'll drop it.

> > +static int jz4740_pwm_remove(struct platform_device *pdev)
> __devexit

Can do.

> > +{
> > +	struct jz4740_pwm_chip *jz4740 = platform_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	ret = pwmchip_remove(&jz4740->chip);
> > +	if (ret < 0)
> > +		return ret;
> 
> remove is not really allowed to fail, the return value is never really tested
> and the device is removed nevertheless. But this seems to be a problem with the
> PWM API. It should be possible to remove a PWM chip even if it is currently in
> use and after a PWM chip has been removed all calls to a pwm_device of that
> chip it should return an error. This will require reference counting for the
> pwm_device struct though. E.g. by adding a 'struct device' to it.

I beg to differ. It shouldn't be possible to remove a PWM chip that
provides requested PWM devices. All other drivers do the same here.

> > +
> > +	clk_put(jz4740->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver jz4740_pwm_driver = {
> > +	.driver = {
> > +		.name = "jz4740-pwm",
> 
> .owner = THIS_MODULE,

There are a number of other drivers where this is missing. I'll make a
note to add it to those as well.

> > +	},
> > +	.probe = jz4740_pwm_probe,
> > +	.remove = jz4740_pwm_remove,
> 
> .remove = __devexit_p(jz4740_pwm_remove),

Yes.

> 
> > +};
> > +module_platform_driver(jz4740_pwm_driver);
> 
> MODULE_LICENSE(...), MODULE_AUTHOR(...), MODULE_DESCRIPTION(...), MODULE_ALIAS(...)

Those weren't present previously. I suppose they should be "GPL", you,
"Ingenic JZ4740 PWM driver" and "platform:jz4740-pwm", respectively?

Thierry

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

Re: [PATCH 3/3] pwm: Add Ingenic JZ4740 support

[Lars-Peter Clausen]

On 09/02/2012 09:59 PM, Thierry Reding wrote:
>>> +	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
>>> +	if (is_enabled)
>>> +		pwm_disable(pwm);
>>
>> I think this should be jz4740_pwm_disable
>>
>>> +
>>> +	jz4740_timer_set_count(pwm->hwpwm, 0);
>>> +	jz4740_timer_set_duty(pwm->hwpwm, duty);
>>> +	jz4740_timer_set_period(pwm->hwpwm, period);
>>> +
>>> +	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
>>> +		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
>>> +
>>> +	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
>>> +
>>> +	if (is_enabled)
>>> +		pwm_enable(pwm);
>>
>> and jz4740_pwm_enable here.
> 
> I wonder if this is actually required here. Can the timer really not be
> reprogrammed while enabled?
>

It can, but we've observed this to cause permanent glitches until the timer is
reprogrammed again.

>>> +{
>>> +	struct jz4740_pwm_chip *jz4740 = platform_get_drvdata(pdev);
>>> +	int ret;
>>> +
>>> +	ret = pwmchip_remove(&jz4740->chip);
>>> +	if (ret < 0)
>>> +		return ret;
>>
>> remove is not really allowed to fail, the return value is never really tested
>> and the device is removed nevertheless. But this seems to be a problem with the
>> PWM API. It should be possible to remove a PWM chip even if it is currently in
>> use and after a PWM chip has been removed all calls to a pwm_device of that
>> chip it should return an error. This will require reference counting for the
>> pwm_device struct though. E.g. by adding a 'struct device' to it.
> 
> I beg to differ. It shouldn't be possible to remove a PWM chip that
> provides requested PWM devices. All other drivers do the same here.

Part of the Linux device driver model is that that a device may appear or
disappear at any given time (if the kernel has been compiled with
CONFIG_HOTPLUG). So you can't prevent removal. The fact that the remove
callback function return an int is kind of misleading and should probably be
fixed at some point. The return value is never checked and the device will be
removed nevertheless. So the PWM subsystem must cope with the case where the
PWM chip is removed while some of its pwm_devices are still in use.

[...]
>>
>>> +};
>>> +module_platform_driver(jz4740_pwm_driver);
>>
>> MODULE_LICENSE(...), MODULE_AUTHOR(...), MODULE_DESCRIPTION(...), MODULE_ALIAS(...)
> 
> Those weren't present previously. I suppose they should be "GPL", you,
> "Ingenic JZ4740 PWM driver" and "platform:jz4740-pwm", respectively?

Yes, sounds good. The old code couldn't be build as a module, so these were not
necessary previously.

Re: [PATCH 3/3] pwm: Add Ingenic JZ4740 support

[Thierry Reding]

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

On Sun, Sep 02, 2012 at 10:22:29PM +0200, Lars-Peter Clausen wrote:
> On 09/02/2012 09:59 PM, Thierry Reding wrote:
> >>> +	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
> >>> +	if (is_enabled)
> >>> +		pwm_disable(pwm);
> >>
> >> I think this should be jz4740_pwm_disable
> >>
> >>> +
> >>> +	jz4740_timer_set_count(pwm->hwpwm, 0);
> >>> +	jz4740_timer_set_duty(pwm->hwpwm, duty);
> >>> +	jz4740_timer_set_period(pwm->hwpwm, period);
> >>> +
> >>> +	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
> >>> +		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
> >>> +
> >>> +	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
> >>> +
> >>> +	if (is_enabled)
> >>> +		pwm_enable(pwm);
> >>
> >> and jz4740_pwm_enable here.
> > 
> > I wonder if this is actually required here. Can the timer really not be
> > reprogrammed while enabled?
> >
> 
> It can, but we've observed this to cause permanent glitches until the timer is
> reprogrammed again.

Okay. I've changed this to use jz4740_pwm_{enable,disable}() instead.

> >>> +{
> >>> +	struct jz4740_pwm_chip *jz4740 = platform_get_drvdata(pdev);
> >>> +	int ret;
> >>> +
> >>> +	ret = pwmchip_remove(&jz4740->chip);
> >>> +	if (ret < 0)
> >>> +		return ret;
> >>
> >> remove is not really allowed to fail, the return value is never really tested
> >> and the device is removed nevertheless. But this seems to be a problem with the
> >> PWM API. It should be possible to remove a PWM chip even if it is currently in
> >> use and after a PWM chip has been removed all calls to a pwm_device of that
> >> chip it should return an error. This will require reference counting for the
> >> pwm_device struct though. E.g. by adding a 'struct device' to it.
> > 
> > I beg to differ. It shouldn't be possible to remove a PWM chip that
> > provides requested PWM devices. All other drivers do the same here.
> 
> Part of the Linux device driver model is that that a device may appear or
> disappear at any given time (if the kernel has been compiled with
> CONFIG_HOTPLUG). So you can't prevent removal. The fact that the remove
> callback function return an int is kind of misleading and should probably be
> fixed at some point. The return value is never checked and the device will be
> removed nevertheless. So the PWM subsystem must cope with the case where the
> PWM chip is removed while some of its pwm_devices are still in use.

I thought I had seen this work. But looking at the code, you're right.
Perhaps what I saw was caused by the reference counting done on the
pwm_ops structure. At least that keeps the module from being unloaded if
there are still any requested PWM devices, but it won't help if the
device suddenly goes away. I wonder if that's a realistic use-case,
though, at least for platform devices.

I currently can't run any tests because I don't have any hardware
available. I'll need to take another look when I'm back at work next
week and think of a way to solve this. Adding some reference counting as
you suggested earlier may be the only way.

Thierry

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

Re: [PATCH 0/3] MIPS: JZ4740: Move PWM driver to PWM framework

[Maarten ter Huurne]

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

On Sunday 02 September 2012 11:52:27 Thierry Reding wrote:

> This small series fixes a build error due to a circular header
> dependency, exports the timer API so it can be used outside of
> the arch/mips/jz4740 tree and finally moves and converts the
> JZ4740 PWM driver to the PWM framework.
> 
> Note that I don't have any hardware to test this on, so I had to
> rely on compile tests only. Patches 1 and 2 should probably go
> through the MIPS tree, while I can take patch 3 through the PWM
> tree. It touches a couple of files in arch/mips but the changes
> are unlikely to cause conflicts.

Exporting the hardware outputs PWM2-7 as index 0-5 in the PWM core is rather 
confusing. I discussed with Lars on IRC and it's probably better to expose 
PWM0-7 through the API, but refuse to hand out PWM0 and PWM1 when requested, 
since their associated timers are in use by the system. I attached a diff 
that illustrates this approach.

Note that if this approach is taken, the beeper ID in board-qi_lb60.c should 
be changed back from 2 to 4, since the beeper is attached to PWM4.

I tested the "for-next" branch on the Dingoo A320 with the pwm-backlight 
driver. It didn't work at first, because the PWM number and the timer number 
didn't align: I requested PWM number 5 to get PWM7 and the GPIO of PWM7 was 
used, but with timer 5 instead of timer 7, resulting in a dark screen. 
However, it works fine after adding PWM0/1 as described above.

If other people want to test on real hardware, you can find the code in 
branch jz-3.6-rc2-pwm in the qi-kernel repository. Unfortunately our web 
interface for git is still broken, but the repo itself is fine.
  git://projects.qi-hardware.com/qi-kernel.git

Bye,
		Maarten

[-- Attachment #2: jz4740-pwm-all-8.diff --]
[-- Type: text/x-patch, Size: 779 bytes --]

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index db29b37..554e414 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -24,9 +24,11 @@
 #include <asm/mach-jz4740/gpio.h>
 #include <timer.h>
 
-#define NUM_PWM 6
+#define NUM_PWM 8
 
 static const unsigned int jz4740_pwm_gpio_list[NUM_PWM] = {
+	JZ_GPIO_PWM0,
+	JZ_GPIO_PWM1,
 	JZ_GPIO_PWM2,
 	JZ_GPIO_PWM3,
 	JZ_GPIO_PWM4,
@@ -50,6 +52,13 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	unsigned int gpio = jz4740_pwm_gpio_list[pwm->hwpwm];
 	int ret;
 
+	/*
+	 * Timer 0 and 1 are used for system tasks, so they are unavailable
+	 * for use as PWMs.
+	 */
+	if (pwm->hwpwm < 2)
+		return -EBUSY;
+
 	ret = gpio_request(gpio, pwm->label);
 
 	if (ret) {

Re: [PATCH 0/3] MIPS: JZ4740: Move PWM driver to PWM framework

[Thierry Reding]

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

On Sun, Sep 02, 2012 at 03:25:55PM +0200, Maarten ter Huurne wrote:
> On Sunday 02 September 2012 11:52:27 Thierry Reding wrote:
> 
> > This small series fixes a build error due to a circular header
> > dependency, exports the timer API so it can be used outside of
> > the arch/mips/jz4740 tree and finally moves and converts the
> > JZ4740 PWM driver to the PWM framework.
> > 
> > Note that I don't have any hardware to test this on, so I had to
> > rely on compile tests only. Patches 1 and 2 should probably go
> > through the MIPS tree, while I can take patch 3 through the PWM
> > tree. It touches a couple of files in arch/mips but the changes
> > are unlikely to cause conflicts.
> 
> Exporting the hardware outputs PWM2-7 as index 0-5 in the PWM core is rather 
> confusing. I discussed with Lars on IRC and it's probably better to expose 
> PWM0-7 through the API, but refuse to hand out PWM0 and PWM1 when requested, 
> since their associated timers are in use by the system. I attached a diff 
> that illustrates this approach.
> 
> Note that if this approach is taken, the beeper ID in board-qi_lb60.c should 
> be changed back from 2 to 4, since the beeper is attached to PWM4.
> 
> I tested the "for-next" branch on the Dingoo A320 with the pwm-backlight 
> driver. It didn't work at first, because the PWM number and the timer number 
> didn't align: I requested PWM number 5 to get PWM7 and the GPIO of PWM7 was 
> used, but with timer 5 instead of timer 7, resulting in a dark screen. 
> However, it works fine after adding PWM0/1 as described above.

I haven't seen any usage of the pwm-backlight driver in mainline. I
assume this is only present in some downstream repository?

> If other people want to test on real hardware, you can find the code in 
> branch jz-3.6-rc2-pwm in the qi-kernel repository. Unfortunately our web 
> interface for git is still broken, but the repo itself is fine.
>   git://projects.qi-hardware.com/qi-kernel.git
> 
> Bye,
> 		Maarten

> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index db29b37..554e414 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -24,9 +24,11 @@
>  #include <asm/mach-jz4740/gpio.h>
>  #include <timer.h>
>  
> -#define NUM_PWM 6
> +#define NUM_PWM 8
>  
>  static const unsigned int jz4740_pwm_gpio_list[NUM_PWM] = {
> +	JZ_GPIO_PWM0,
> +	JZ_GPIO_PWM1,
>  	JZ_GPIO_PWM2,
>  	JZ_GPIO_PWM3,
>  	JZ_GPIO_PWM4,
> @@ -50,6 +52,13 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  	unsigned int gpio = jz4740_pwm_gpio_list[pwm->hwpwm];
>  	int ret;
>  
> +	/*
> +	 * Timer 0 and 1 are used for system tasks, so they are unavailable
> +	 * for use as PWMs.
> +	 */
> +	if (pwm->hwpwm < 2)
> +		return -EBUSY;
> +
>  	ret = gpio_request(gpio, pwm->label);
>  
>  	if (ret) {

An alternative approach would be to change pwm_chip.base from -1
(dynamically allocated) to 2, which would leave 0 and 1 unavailable.
That should at least solve the problem that you had regarding the GPIO
and timer mismatch.

But the above also sounds sensible, and since both you and Lars agree
that this is the better option, I can squash these changes into my patch
with your permission.

Thierry

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

Re: [PATCH 0/3] MIPS: JZ4740: Move PWM driver to PWM framework

[Maarten ter Huurne]

On Sunday 02 September 2012 21:27:53 Thierry Reding wrote:
> On Sun, Sep 02, 2012 at 03:25:55PM +0200, Maarten ter Huurne wrote:
> > I tested the "for-next" branch on the Dingoo A320 with the pwm-backlight
> > driver. It didn't work at first, because the PWM number and the timer
> > number didn't align: I requested PWM number 5 to get PWM7 and the GPIO
> > of PWM7 was used, but with timer 5 instead of timer 7, resulting in a
> > dark screen. However, it works fine after adding PWM0/1 as described
> > above.
> 
> I haven't seen any usage of the pwm-backlight driver in mainline. I
> assume this is only present in some downstream repository?

Yes, the Dingoo A320 support is currently only available in the qi-kernel 
repository. We have some essential drivers (the SLCD framebuffer driver in 
particular) that are in their current state just too ugly to submit to 
mainline.

> > If other people want to test on real hardware, you can find the code in
> > branch jz-3.6-rc2-pwm in the qi-kernel repository. Unfortunately our web
> > interface for git is still broken, but the repo itself is fine.
> > 
> >   git://projects.qi-hardware.com/qi-kernel.git

This is where you can find the code. The relevant configs are 
qi_lb60_defconfig and a320_defconfig.

> An alternative approach would be to change pwm_chip.base from -1
> (dynamically allocated) to 2, which would leave 0 and 1 unavailable.
> That should at least solve the problem that you had regarding the GPIO
> and timer mismatch.

That could work, but the hardware does have PWM0 and PWM1, which are just 
not available in our kernel, so adding them in busy state would better 
describe real situation.

Maybe at some point we'll have a generic timer framework as well and then 
having PWM0/1 defined but not requestable because the timers are busy would 
be a natural fit.

> But the above also sounds sensible, and since both you and Lars agree
> that this is the better option, I can squash these changes into my patch
> with your permission.

Yes, please do.

Bye,
		Maarten