Re: [PATCH v3 2/3] pwm: New driver to support PWMs on TWL4030/6030 series of PMICs
On 11/23/2012 03:58 PM, Thierry Reding wrote:
On Tue, Nov 20, 2012 at 10:56:21AM +0100, Peter Ujfalusi wrote:
The driver supports the following PWM outputs:
TWL4030 PWM0 and PWM1
TWL6030 PWM1 and PWM2
On TWL4030 the PWM signals are muxed. Upon requesting the PWM the driver
will select the correct mux so the PWM can be used. When the PWM has been
freed the original configuration going to be restored.
configuration is going to be
Yes, I'll fix it.
+config PWM_TWL
+tristate TWL4030/6030 PWM support
+select HAVE_PWM
Why do you select HAVE_PWM?
LEDS_PWM driver for example depends on this option. Not sure why we have this
in the first place, why PWM alone is not good enough?
we could also select HAVE_PWM when the CONFIG_PWM is enabled to cut back on
duplicated lines in the Kconfig.
+static int twl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+int duty_cycle = DIV_ROUND_UP(duty_ns * TWL_PWM_MAX, period_ns);
+u8 pwm_config[2] = {1, 0};
+int base, ret;
+
+/*
+ * To configure the duty period:
+ * On-cycle is set to 1 (the minimum allowed value)
+ * The off time of 0 is not configurable, so the mapping is:
+ * 0 - off cycle = 2,
+ * 1 - off cycle = 2,
+ * 2 - off cycle = 3,
+ * 126 - off cycle 127,
+ * 127 - off cycle 1
+ * When on cycle == off cycle the PWM will be always on
+ */
+duty_cycle += 1;
The canonical form to write this would be duty_cycle++, but maybe it
would even be better to add it to the expression that defines
duty_cycle?
True. This is actually a leftover from a previous version I had for the
calculation. Will change it.
+static int twl4030_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+int ret;
+u8 val;
+
+ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+if (ret 0) {
+dev_err(chip-dev, %s: Failed to read GPBR1\n, pwm-label);
+return ret;
+}
+
+val |= TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMXCLK_ENABLE);
+
+ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+if (ret 0)
+dev_err(chip-dev, %s: Failed to enable PWM\n, pwm-label);
+
+val |= TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMX_ENABLE);
+
+ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+if (ret 0)
+dev_err(chip-dev, %s: Failed to enable PWM\n, pwm-label);
+
+return ret;
+}
Does this perhaps need some locking to make sure the read-modify-write
sequence isn't interrupted?
I'll add locking (mutex) to both drivers.
+static void twl4030_pwm_disable(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
+int ret;
+u8 val;
+
+ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+if (ret 0) {
+dev_err(chip-dev, %s: Failed to read GPBR1\n, pwm-label);
+return;
+}
+
+val = ~TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMX_ENABLE);
+
+ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+if (ret 0)
+dev_err(chip-dev, %s: Failed to disable PWM\n, pwm-label);
+
+val = ~TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMXCLK_ENABLE);
+
+ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+if (ret 0)
+dev_err(chip-dev, %s: Failed to disable PWM\n, pwm-label);
+}
Same here.
+static int twl4030_pwm_request(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
+struct twl_pwm_chip *twl = container_of(chip, struct twl_pwm_chip,
+chip);
This could use a macro like to_twl(chip).
Does the macro really makes it more readable?
+int ret;
+u8 val, mask, bits;
+
+ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
+if (ret 0) {
+dev_err(chip-dev, %s: Failed to read PMBR1\n, pwm-label);
+return ret;
+}
+
+if (pwm-hwpwm) {
+/* PWM 1 */
+mask = TWL4030_GPIO7_VIBRASYNC_PWM1_MASK;
+bits = TWL4030_GPIO7_VIBRASYNC_PWM1_PWM1;
+} else {
+/* PWM 0 */
+mask = TWL4030_GPIO6_PWM0_MUTE_MASK;
+bits = TWL4030_GPIO6_PWM0_MUTE_PWM0;
+}
+
+/* Save the current MUX configuration for the PWM */
+twl-twl4030_pwm_mux = ~mask;
+twl-twl4030_pwm_mux |= (val mask);
+
+/* Select PWM functionality */
+val = ~mask;
+val |= bits;
+
+ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
+if (ret 0)
+dev_err(chip-dev, %s: Failed to request PWM\n, pwm-label);
+
+return ret;
+}
Again, more read-modify-write without locking.
+
+static void twl4030_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+struct twl_pwm_chip *twl = container_of(chip,
Re: [PATCH v3 2/3] pwm: New driver to support PWMs on TWL4030/6030 series of PMICs
On Tue, Nov 20, 2012 at 10:56:21AM +0100, Peter Ujfalusi wrote:
The driver supports the following PWM outputs:
TWL4030 PWM0 and PWM1
TWL6030 PWM1 and PWM2
On TWL4030 the PWM signals are muxed. Upon requesting the PWM the driver
will select the correct mux so the PWM can be used. When the PWM has been
freed the original configuration going to be restored.
configuration is going to be
+config PWM_TWL
+ tristate TWL4030/6030 PWM support
+ select HAVE_PWM
Why do you select HAVE_PWM?
+static int twl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ int duty_cycle = DIV_ROUND_UP(duty_ns * TWL_PWM_MAX, period_ns);
+ u8 pwm_config[2] = {1, 0};
+ int base, ret;
+
+ /*
+ * To configure the duty period:
+ * On-cycle is set to 1 (the minimum allowed value)
+ * The off time of 0 is not configurable, so the mapping is:
+ * 0 - off cycle = 2,
+ * 1 - off cycle = 2,
+ * 2 - off cycle = 3,
+ * 126 - off cycle 127,
+ * 127 - off cycle 1
+ * When on cycle == off cycle the PWM will be always on
+ */
+ duty_cycle += 1;
The canonical form to write this would be duty_cycle++, but maybe it
would even be better to add it to the expression that defines
duty_cycle?
+static int twl4030_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ int ret;
+ u8 val;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+ if (ret 0) {
+ dev_err(chip-dev, %s: Failed to read GPBR1\n, pwm-label);
+ return ret;
+ }
+
+ val |= TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMXCLK_ENABLE);
+
+ ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+ if (ret 0)
+ dev_err(chip-dev, %s: Failed to enable PWM\n, pwm-label);
+
+ val |= TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMX_ENABLE);
+
+ ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+ if (ret 0)
+ dev_err(chip-dev, %s: Failed to enable PWM\n, pwm-label);
+
+ return ret;
+}
Does this perhaps need some locking to make sure the read-modify-write
sequence isn't interrupted?
+static void twl4030_pwm_disable(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
+ int ret;
+ u8 val;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+ if (ret 0) {
+ dev_err(chip-dev, %s: Failed to read GPBR1\n, pwm-label);
+ return;
+ }
+
+ val = ~TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMX_ENABLE);
+
+ ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+ if (ret 0)
+ dev_err(chip-dev, %s: Failed to disable PWM\n, pwm-label);
+
+ val = ~TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMXCLK_ENABLE);
+
+ ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
+ if (ret 0)
+ dev_err(chip-dev, %s: Failed to disable PWM\n, pwm-label);
+}
Same here.
+static int twl4030_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct twl_pwm_chip *twl = container_of(chip, struct twl_pwm_chip,
+ chip);
This could use a macro like to_twl(chip).
+ int ret;
+ u8 val, mask, bits;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
+ if (ret 0) {
+ dev_err(chip-dev, %s: Failed to read PMBR1\n, pwm-label);
+ return ret;
+ }
+
+ if (pwm-hwpwm) {
+ /* PWM 1 */
+ mask = TWL4030_GPIO7_VIBRASYNC_PWM1_MASK;
+ bits = TWL4030_GPIO7_VIBRASYNC_PWM1_PWM1;
+ } else {
+ /* PWM 0 */
+ mask = TWL4030_GPIO6_PWM0_MUTE_MASK;
+ bits = TWL4030_GPIO6_PWM0_MUTE_PWM0;
+ }
+
+ /* Save the current MUX configuration for the PWM */
+ twl-twl4030_pwm_mux = ~mask;
+ twl-twl4030_pwm_mux |= (val mask);
+
+ /* Select PWM functionality */
+ val = ~mask;
+ val |= bits;
+
+ ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
+ if (ret 0)
+ dev_err(chip-dev, %s: Failed to request PWM\n, pwm-label);
+
+ return ret;
+}
Again, more read-modify-write without locking.
+
+static void twl4030_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct twl_pwm_chip *twl = container_of(chip, struct twl_pwm_chip,
+ chip);
+ int ret;
+ u8 val, mask;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
+ if (ret 0) {
+ dev_err(chip-dev, %s: Failed to read PMBR1\n, pwm-label);
+ return;
+ }
+
+ if (pwm-hwpwm)
+ /* PWM 1 */
+ mask = TWL4030_GPIO7_VIBRASYNC_PWM1_MASK;
+ else
+
[PATCH v3 2/3] pwm: New driver to support PWMs on TWL4030/6030 series of PMICs
The driver supports the following PWM outputs:
TWL4030 PWM0 and PWM1
TWL6030 PWM1 and PWM2
On TWL4030 the PWM signals are muxed. Upon requesting the PWM the driver
will select the correct mux so the PWM can be used. When the PWM has been
freed the original configuration going to be restored.
Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
---
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-twl.c | 330 ++
3 files changed, 341 insertions(+)
create mode 100644 drivers/pwm/pwm-twl.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e678005..c577db9 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -142,6 +142,16 @@ config PWM_TIEHRPWM
To compile this driver as a module, choose M here: the module
will be called pwm-tiehrpwm.
+config PWM_TWL
+ tristate TWL4030/6030 PWM support
+ select HAVE_PWM
+ depends on TWL4030_CORE
+ help
+ Generic PWM framework driver for TWL4030/6030.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-twl.
+
config PWM_VT8500
tristate vt8500 pwm support
depends on ARCH_VT8500
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 29cf57e..3324c06 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -11,4 +11,5 @@ obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
obj-$(CONFIG_PWM_TEGRA)+= pwm-tegra.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
+obj-$(CONFIG_PWM_TWL) += pwm-twl.o
obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
diff --git a/drivers/pwm/pwm-twl.c b/drivers/pwm/pwm-twl.c
new file mode 100644
index 000..fadf126
--- /dev/null
+++ b/drivers/pwm/pwm-twl.c
@@ -0,0 +1,330 @@
+/*
+ * Driver for TWL4030/6030 Generic Pulse Width Modulator
+ *
+ * Copyright (C) 2012 Texas Instruments
+ * Author: Peter Ujfalusi peter.ujfal...@ti.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see http://www.gnu.org/licenses/.
+ */
+
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/pwm.h
+#include linux/i2c/twl.h
+#include linux/slab.h
+
+/*
+ * This driver handles the PWMs of TWL4030 and TWL6030.
+ * The TRM names for the PWMs on TWL4030 are: PWM0, PWM1
+ * TWL6030 also have two PWMs named in the TRM as PWM1, PWM2
+ */
+
+#define TWL_PWM_MAX0x7f
+
+/* Registers, bits and macro for TWL4030 */
+#define TWL4030_GPBR1_REG 0x0c
+#define TWL4030_PMBR1_REG 0x0d
+
+/* GPBR1 register bits */
+#define TWL4030_PWMXCLK_ENABLE (1 0)
+#define TWL4030_PWMX_ENABLE(1 2)
+#define TWL4030_PWMX_BITS (TWL4030_PWMX_ENABLE | TWL4030_PWMXCLK_ENABLE)
+#define TWL4030_PWM_TOGGLE(pwm, x) ((x) (pwm))
+
+/* PMBR1 register bits */
+#define TWL4030_GPIO6_PWM0_MUTE_MASK (0x03 2)
+#define TWL4030_GPIO6_PWM0_MUTE_PWM0 (0x01 2)
+#define TWL4030_GPIO7_VIBRASYNC_PWM1_MASK (0x03 4)
+#define TWL4030_GPIO7_VIBRASYNC_PWM1_PWM1 (0x03 4)
+
+/* Register, bits and macro for TWL6030 */
+#define TWL6030_TOGGLE3_REG0x92
+
+#define TWL6030_PWMXR (1 0)
+#define TWL6030_PWMXS (1 1)
+#define TWL6030_PWMXEN (1 2)
+#define TWL6030_PWM_TOGGLE(pwm, x) ((x) (pwm * 3))
+
+struct twl_pwm_chip {
+ struct pwm_chip chip;
+ u8 twl6030_toggle3;
+ u8 twl4030_pwm_mux;
+};
+
+static int twl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ int duty_cycle = DIV_ROUND_UP(duty_ns * TWL_PWM_MAX, period_ns);
+ u8 pwm_config[2] = {1, 0};
+ int base, ret;
+
+ /*
+* To configure the duty period:
+* On-cycle is set to 1 (the minimum allowed value)
+* The off time of 0 is not configurable, so the mapping is:
+* 0 - off cycle = 2,
+* 1 - off cycle = 2,
+* 2 - off cycle = 3,
+* 126 - off cycle 127,
+* 127 - off cycle 1
+* When on cycle == off cycle the PWM will be always on
+*/
+ duty_cycle += 1;
+ if (duty_cycle == 1)
+ duty_cycle = 2;
+ else if (duty_cycle TWL_PWM_MAX)
+ duty_cycle = 1;
+
+ base = pwm-hwpwm * 3;
+
+ pwm_config[1] = duty_cycle;
+
+ ret = twl_i2c_write(TWL_MODULE_PWM, pwm_config, base, 2);
+ if (ret 0)
+
