[PATCH v3 0/3] pwm: Drivers for twl4030/6030 PWMs and LEDs

[PATCH v3 0/3] pwm: Drivers for twl4030/6030 PWMs and LEDs

[Peter Ujfalusi]

Hello,

Changes since v2:
- New PWM cycle calculation for twl-pwm driver and twl4030's pwm-twl-led with
  comment to document the expected behaviour.

Changes cince v1:
- The turn on/off sequence has been corrected for twl4030 PWMs as suggested by
  Grazvydas Ignotas
- Comment section added to the new drivers to describe what they are controlling

The series has been tested (with additional, pending patches):
Zoom2 for twl4030 PWM0 (keypad light), PWM1 (LCD backlight)
BeagleBoard for twl4030 LEDB (PWMB - pmu_stat led)
OMAP4 Blaze (SDP4430) for twl6030 PWM1 (keypad light), PWM2 (LCD backlight) and
LED (charging indication led).

Intro mail from v1:

The currently available pwm-twl6030.c driver only supports TWL6030's Charging
indication LED.
Remove this driver and add two new ones which implements support for all PWM
driven outputs:
pwm-twl driver supports twl4030 (PWM 0/1) and twl6030 (PWM 1/2) instances
pwm-twl-led driver supports twl4030 (PWM driven LED A/B ports) and twl6030's
Charging indication LED (PWM driven).

Regards,
Peter
---
Peter Ujfalusi (3):
  pwm: Remove pwm-twl6030 driver
  pwm: New driver to support PWMs on TWL4030/6030 series of PMICs
  pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of
    PMICs

 drivers/pwm/Kconfig       |  19 ++-
 drivers/pwm/Makefile      |   3 +-
 drivers/pwm/pwm-twl-led.c | 303 ++++++++++++++++++++++++++++++++++++++++++
 drivers/pwm/pwm-twl.c     | 330 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pwm/pwm-twl6030.c | 184 --------------------------
 5 files changed, 650 insertions(+), 189 deletions(-)
 create mode 100644 drivers/pwm/pwm-twl-led.c
 create mode 100644 drivers/pwm/pwm-twl.c
 delete mode 100644 drivers/pwm/pwm-twl6030.c

-- 
1.8.0

[PATCH v3 1/3] pwm: Remove pwm-twl6030 driver

[Peter Ujfalusi]

This driver only supported the Charging indicator LED.
New set of drivers going to provide support for both PWMs and LEDs for twl4030
and twl6030 series of PMICs.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/pwm/Kconfig       |   9 ---
 drivers/pwm/Makefile      |   1 -
 drivers/pwm/pwm-twl6030.c | 184 ----------------------------------------------
 3 files changed, 194 deletions(-)
 delete mode 100644 drivers/pwm/pwm-twl6030.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..e678005 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -142,15 +142,6 @@ config  PWM_TIEHRPWM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-tiehrpwm.
 
-config PWM_TWL6030
-	tristate "TWL6030 PWM support"
-	depends on TWL4030_CORE
-	help
-	  Generic PWM framework driver for TWL6030.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called pwm-twl6030.
-
 config PWM_VT8500
 	tristate "vt8500 pwm support"
 	depends on ARCH_VT8500
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..29cf57e 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -11,5 +11,4 @@ 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_TWL6030)	+= pwm-twl6030.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
diff --git a/drivers/pwm/pwm-twl6030.c b/drivers/pwm/pwm-twl6030.c
deleted file mode 100644
index 8e63878..0000000
--- a/drivers/pwm/pwm-twl6030.c
+++ /dev/null
@@ -1,184 +0,0 @@
-/*
- * twl6030_pwm.c
- * Driver for PHOENIX (TWL6030) Pulse Width Modulator
- *
- * Copyright (C) 2010 Texas Instruments
- * Author: Hemanth V <hemanthv@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>
-
-#define LED_PWM_CTRL1	0xF4
-#define LED_PWM_CTRL2	0xF5
-
-/* Max value for CTRL1 register */
-#define PWM_CTRL1_MAX	255
-
-/* Pull down disable */
-#define PWM_CTRL2_DIS_PD	(1 << 6)
-
-/* Current control 2.5 milli Amps */
-#define PWM_CTRL2_CURR_02	(2 << 4)
-
-/* LED supply source */
-#define PWM_CTRL2_SRC_VAC	(1 << 2)
-
-/* LED modes */
-#define PWM_CTRL2_MODE_HW	(0 << 0)
-#define PWM_CTRL2_MODE_SW	(1 << 0)
-#define PWM_CTRL2_MODE_DIS	(2 << 0)
-
-#define PWM_CTRL2_MODE_MASK	0x3
-
-struct twl6030_pwm_chip {
-	struct pwm_chip chip;
-};
-
-static int twl6030_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	int ret;
-	u8 val;
-
-	/* Configure PWM */
-	val = PWM_CTRL2_DIS_PD | PWM_CTRL2_CURR_02 | PWM_CTRL2_SRC_VAC |
-	      PWM_CTRL2_MODE_HW;
-
-	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, LED_PWM_CTRL2);
-	if (ret < 0) {
-		dev_err(chip->dev, "%s: Failed to configure PWM, Error %d\n",
-			pwm->label, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
-static int twl6030_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			      int duty_ns, int period_ns)
-{
-	u8 duty_cycle = (duty_ns * PWM_CTRL1_MAX) / period_ns;
-	int ret;
-
-	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, duty_cycle, LED_PWM_CTRL1);
-	if (ret < 0) {
-		pr_err("%s: Failed to configure PWM, Error %d\n",
-			pwm->label, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
-static int twl6030_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	int ret;
-	u8 val;
-
-	ret = twl_i2c_read_u8(TWL6030_MODULE_ID1, &val, LED_PWM_CTRL2);
-	if (ret < 0) {
-		dev_err(chip->dev, "%s: Failed to enable PWM, Error %d\n",
-			pwm->label, ret);
-		return ret;
-	}
-
-	/* Change mode to software control */
-	val &= ~PWM_CTRL2_MODE_MASK;
-	val |= PWM_CTRL2_MODE_SW;
-
-	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, LED_PWM_CTRL2);
-	if (ret < 0) {
-		dev_err(chip->dev, "%s: Failed to enable PWM, Error %d\n",
-			pwm->label, ret);
-		return ret;
-	}
-
-	twl_i2c_read_u8(TWL6030_MODULE_ID1, &val, LED_PWM_CTRL2);
-	return 0;
-}
-
-static void twl6030_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	int ret;
-	u8 val;
-
-	ret = twl_i2c_read_u8(TWL6030_MODULE_ID1, &val, LED_PWM_CTRL2);
-	if (ret < 0) {
-		dev_err(chip->dev, "%s: Failed to disable PWM, Error %d\n",
-			pwm->label, ret);
-		return;
-	}
-
-	val &= ~PWM_CTRL2_MODE_MASK;
-	val |= PWM_CTRL2_MODE_HW;
-
-	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, LED_PWM_CTRL2);
-	if (ret < 0) {
-		dev_err(chip->dev, "%s: Failed to disable PWM, Error %d\n",
-			pwm->label, ret);
-	}
-}
-
-static const struct pwm_ops twl6030_pwm_ops = {
-	.request = twl6030_pwm_request,
-	.config = twl6030_pwm_config,
-	.enable = twl6030_pwm_enable,
-	.disable = twl6030_pwm_disable,
-};
-
-static int twl6030_pwm_probe(struct platform_device *pdev)
-{
-	struct twl6030_pwm_chip *twl6030;
-	int ret;
-
-	twl6030 = devm_kzalloc(&pdev->dev, sizeof(*twl6030), GFP_KERNEL);
-	if (!twl6030)
-		return -ENOMEM;
-
-	twl6030->chip.dev = &pdev->dev;
-	twl6030->chip.ops = &twl6030_pwm_ops;
-	twl6030->chip.base = -1;
-	twl6030->chip.npwm = 1;
-
-	ret = pwmchip_add(&twl6030->chip);
-	if (ret < 0)
-		return ret;
-
-	platform_set_drvdata(pdev, twl6030);
-
-	return 0;
-}
-
-static int twl6030_pwm_remove(struct platform_device *pdev)
-{
-	struct twl6030_pwm_chip *twl6030 = platform_get_drvdata(pdev);
-
-	return pwmchip_remove(&twl6030->chip);
-}
-
-static struct platform_driver twl6030_pwm_driver = {
-	.driver = {
-		.name = "twl6030-pwm",
-	},
-	.probe = twl6030_pwm_probe,
-	.remove = __devexit_p(twl6030_pwm_remove),
-};
-module_platform_driver(twl6030_pwm_driver);
-
-MODULE_ALIAS("platform:twl6030-pwm");
-MODULE_LICENSE("GPL");
-- 
1.8.0

[PATCH v3 2/3] pwm: New driver to support PWMs on TWL4030/6030 series of PMICs

[Peter Ujfalusi]

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.ujfalusi@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 0000000..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.ujfalusi@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_MAX		0x7f
+
+/* 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_REG	0x92
+
+#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)
+		dev_err(chip->dev, "%s: Failed to configure PWM\n", pwm->label);
+
+	return ret;
+}
+
+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;
+}
+
+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);
+}
+
+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);
+	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;
+}
+
+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
+		/* PWM 0 */
+		mask = TWL4030_GPIO6_PWM0_MUTE_MASK;
+
+	/* Restore the MUX configuration for the PWM */
+	val &= ~mask;
+	val |= (twl->twl4030_pwm_mux & mask);
+
+	ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
+	if (ret < 0)
+		dev_err(chip->dev, "%s: Failed to free PWM\n", pwm->label);
+}
+
+static int twl6030_pwm_enable(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;
+
+	val = twl->twl6030_toggle3;
+	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
+	val &= ~TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXR);
+
+	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s: Failed to enable PWM\n", pwm->label);
+		return ret;
+	}
+
+	twl->twl6030_toggle3 = val;
+
+	return 0;
+}
+
+static void twl6030_pwm_disable(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;
+
+	val = twl->twl6030_toggle3;
+	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXR);
+	val &= ~TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
+
+	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s: Failed to read TOGGLE3\n", pwm->label);
+		return;
+	}
+
+	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
+
+	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
+		return;
+	}
+
+	twl->twl6030_toggle3 = val;
+}
+
+static struct pwm_ops twl_pwm_ops = {
+	.config = twl_pwm_config,
+};
+
+static int twl_pwm_probe(struct platform_device *pdev)
+{
+	struct twl_pwm_chip *twl;
+	int ret;
+
+	twl = devm_kzalloc(&pdev->dev, sizeof(*twl), GFP_KERNEL);
+	if (!twl)
+		return -ENOMEM;
+
+	if (twl_class_is_4030()) {
+		twl_pwm_ops.enable = twl4030_pwm_enable;
+		twl_pwm_ops.disable = twl4030_pwm_disable;
+		twl_pwm_ops.request = twl4030_pwm_request;
+		twl_pwm_ops.free = twl4030_pwm_free;
+	} else {
+		twl_pwm_ops.enable = twl6030_pwm_enable;
+		twl_pwm_ops.disable = twl6030_pwm_disable;
+	}
+
+	twl->chip.dev = &pdev->dev;
+	twl->chip.ops = &twl_pwm_ops;
+	twl->chip.base = -1;
+	twl->chip.npwm = 2;
+
+	ret = pwmchip_add(&twl->chip);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, twl);
+
+	return 0;
+}
+
+static int twl_pwm_remove(struct platform_device *pdev)
+{
+	struct twl_pwm_chip *twl = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&twl->chip);
+}
+
+static struct of_device_id twl_pwm_of_match[] = {
+	{ .compatible = "ti,twl4030-pwm" },
+	{ .compatible = "ti,twl6030-pwm" },
+};
+
+MODULE_DEVICE_TABLE(of, twl_pwm_of_match);
+
+static struct platform_driver twl_pwm_driver = {
+	.driver = {
+		.name = "twl-pwm",
+		.of_match_table = of_match_ptr(twl_pwm_of_match),
+	},
+	.probe = twl_pwm_probe,
+	.remove = __devexit_p(twl_pwm_remove),
+};
+module_platform_driver(twl_pwm_driver);
+
+MODULE_AUTHOR("Peter Ujfalusi <peter.ujfalusi@ti.com>");
+MODULE_DESCRIPTION("PWM driver for TWL4030 and TWL6030");
+MODULE_ALIAS("platform:twl-pwm");
+MODULE_LICENSE("GPL");
-- 
1.8.0

[PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs

[Peter Ujfalusi]

The driver supports the following LED outputs as generic PWM driver:
TWL4030 LEDA and LEDB (PWMA and PWMB)
TWL6030 Charging indicator LED (PWM LED)

On TWL6030 when the PWM requested LED is configured to be controlled by SW.
In this case the user can enable/disable and set the duty period freely.
When the PWM has been freed, the LED driver is put back to HW control.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/pwm/Kconfig       |  10 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-twl-led.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 314 insertions(+)
 create mode 100644 drivers/pwm/pwm-twl-led.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index c577db9..49c2082 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -152,6 +152,16 @@ config PWM_TWL
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-twl.
 
+config PWM_TWL_LED
+	tristate "TWL4030/6030 PWM support for LED drivers"
+	select HAVE_PWM
+	depends on TWL4030_CORE
+	help
+	  Generic PWM framework driver for TWL4030/6030 LED.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-twl-led.
+
 config PWM_VT8500
 	tristate "vt8500 pwm support"
 	depends on ARCH_VT8500
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 3324c06..5f26134 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -12,4 +12,5 @@ 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_TWL_LED)	+= pwm-twl-led.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
diff --git a/drivers/pwm/pwm-twl-led.c b/drivers/pwm/pwm-twl-led.c
new file mode 100644
index 0000000..9468720
--- /dev/null
+++ b/drivers/pwm/pwm-twl-led.c
@@ -0,0 +1,303 @@
+/*
+ * Driver for TWL4030/6030 Pulse Width Modulator used as LED driver
+ *
+ * Copyright (C) 2012 Texas Instruments
+ * Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
+ *
+ * This driver is a complete rewrite of the former pwm-twl6030.c authorded by:
+ * Hemanth V <hemanthv@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 PWM driven LED terminals of TWL4030 and TWL6030.
+ * To generate the signal on TWL4030:
+ *  - LEDA uses PWMA
+ *  - LEDB uses PWMB
+ * TWL6030 has one LED pin with dedicated LEDPWM
+ */
+
+#define TWL4030_LED_MAX		0x7f
+#define TWL6030_LED_MAX		0xff
+
+/* Registers, bits and macro for TWL4030 */
+#define TWL4030_LEDEN_REG	0x00
+#define TWL4030_PWMA_REG	0x01
+
+#define TWL4030_LEDXON		(1 << 0)
+#define TWL4030_LEDXPWM		(1 << 4)
+#define TWL4030_LED_PINS	(TWL4030_LEDXON | TWL4030_LEDXPWM)
+#define TWL4030_LED_TOGGLE(led, x)	((x) << (led))
+
+/* Register, bits and macro for TWL6030 */
+#define TWL6030_LED_PWM_CTRL1	0xf4
+#define TWL6030_LED_PWM_CTRL2	0xf5
+
+#define TWL6040_LED_MODE_HW	0x00
+#define TWL6040_LED_MODE_ON	0x01
+#define TWL6040_LED_MODE_OFF	0x02
+#define TWL6040_LED_MODE_MASK	0x03
+
+struct twl_pwmled_chip {
+	struct pwm_chip chip;
+};
+
+static int twl4030_pwmled_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			      int duty_ns, int period_ns)
+{
+	int duty_cycle = DIV_ROUND_UP(duty_ns * TWL4030_LED_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,
+	 * 125 - > off cycle 127,
+	 * 126 - > 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 > TWL4030_LED_MAX)
+		duty_cycle = 1;
+
+	base = pwm->hwpwm * 2 + TWL4030_PWMA_REG;
+
+	pwm_config[1] = duty_cycle;
+
+	ret = twl_i2c_write(TWL4030_MODULE_LED, pwm_config, base, 2);
+	if (ret < 0)
+		dev_err(chip->dev, "%s: Failed to configure PWM\n", pwm->label);
+
+	return ret;
+}
+
+static int twl4030_pwmled_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	int ret;
+	u8 val;
+
+	ret = twl_i2c_read_u8(TWL4030_MODULE_LED, &val, TWL4030_LEDEN_REG);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s: Failed to read LEDEN\n", pwm->label);
+		return ret;
+	}
+
+	val |= TWL4030_LED_TOGGLE(pwm->hwpwm, TWL4030_LED_PINS);
+
+	ret = twl_i2c_write_u8(TWL4030_MODULE_LED, val, TWL4030_LEDEN_REG);
+	if (ret < 0)
+		dev_err(chip->dev, "%s: Failed to enable PWM\n", pwm->label);
+
+	return ret;
+}
+
+static void twl4030_pwmled_disable(struct pwm_chip *chip,
+				   struct pwm_device *pwm)
+{
+	int ret;
+	u8 val;
+
+	ret = twl_i2c_read_u8(TWL4030_MODULE_LED, &val, TWL4030_LEDEN_REG);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s: Failed to read LEDEN\n", pwm->label);
+		return;
+	}
+
+	val &= ~TWL4030_LED_TOGGLE(pwm->hwpwm, TWL4030_LED_PINS);
+
+	ret = twl_i2c_write_u8(TWL4030_MODULE_LED, val, TWL4030_LEDEN_REG);
+	if (ret < 0)
+		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
+}
+
+static int twl6030_pwmled_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			      int duty_ns, int period_ns)
+{
+	int duty_cycle = (duty_ns * TWL6030_LED_MAX) / period_ns;
+	u8 on_time;
+	int ret;
+
+	on_time = duty_cycle & 0xff;
+
+	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, on_time,
+			       TWL6030_LED_PWM_CTRL1);
+	if (ret < 0)
+		dev_err(chip->dev, "%s: Failed to configure PWM\n", pwm->label);
+
+	return ret;
+}
+
+static int twl6030_pwmled_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	int ret;
+	u8 val;
+
+	ret = twl_i2c_read_u8(TWL6030_MODULE_ID1, &val, TWL6030_LED_PWM_CTRL2);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s: Failed to read PWM_CTRL2\n",
+			pwm->label);
+		return ret;
+	}
+
+	val &= ~TWL6040_LED_MODE_MASK;
+	val |= TWL6040_LED_MODE_ON;
+
+	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_LED_PWM_CTRL2);
+	if (ret < 0)
+		dev_err(chip->dev, "%s: Failed to enable PWM\n", pwm->label);
+
+	return ret;
+}
+
+static void twl6030_pwmled_disable(struct pwm_chip *chip,
+				   struct pwm_device *pwm)
+{
+	int ret;
+	u8 val;
+
+	ret = twl_i2c_read_u8(TWL6030_MODULE_ID1, &val, TWL6030_LED_PWM_CTRL2);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s: Failed to read PWM_CTRL2\n",
+			pwm->label);
+		return;
+	}
+
+	val &= ~TWL6040_LED_MODE_MASK;
+	val |= TWL6040_LED_MODE_OFF;
+
+	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_LED_PWM_CTRL2);
+	if (ret < 0)
+		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
+}
+
+static int twl6030_pwmled_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	int ret;
+	u8 val;
+
+	ret = twl_i2c_read_u8(TWL6030_MODULE_ID1, &val, TWL6030_LED_PWM_CTRL2);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s: Failed to read PWM_CTRL2\n",
+			pwm->label);
+		return ret;
+	}
+
+	val &= ~TWL6040_LED_MODE_MASK;
+	val |= TWL6040_LED_MODE_OFF;
+
+	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_LED_PWM_CTRL2);
+	if (ret < 0)
+		dev_err(chip->dev, "%s: Failed to request PWM\n", pwm->label);
+
+	return ret;
+}
+
+static void twl6030_pwmled_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	int ret;
+	u8 val;
+
+	ret = twl_i2c_read_u8(TWL6030_MODULE_ID1, &val, TWL6030_LED_PWM_CTRL2);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s: Failed to read PWM_CTRL2\n",
+			pwm->label);
+		return;
+	}
+
+	val &= ~TWL6040_LED_MODE_MASK;
+	val |= TWL6040_LED_MODE_HW;
+
+	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_LED_PWM_CTRL2);
+	if (ret < 0)
+		dev_err(chip->dev, "%s: Failed to free PWM\n", pwm->label);
+}
+
+static struct pwm_ops twl_pwmled_ops;
+
+static int twl_pwmled_probe(struct platform_device *pdev)
+{
+	struct twl_pwmled_chip *twl;
+	int ret;
+
+	twl = devm_kzalloc(&pdev->dev, sizeof(*twl), GFP_KERNEL);
+	if (!twl)
+		return -ENOMEM;
+
+	if (twl_class_is_4030()) {
+		twl_pwmled_ops.enable = twl4030_pwmled_enable;
+		twl_pwmled_ops.disable = twl4030_pwmled_disable;
+		twl_pwmled_ops.config = twl4030_pwmled_config;
+		twl->chip.npwm = 2;
+	} else {
+		twl_pwmled_ops.enable = twl6030_pwmled_enable;
+		twl_pwmled_ops.disable = twl6030_pwmled_disable;
+		twl_pwmled_ops.config = twl6030_pwmled_config;
+		twl_pwmled_ops.request = twl6030_pwmled_request;
+		twl_pwmled_ops.free = twl6030_pwmled_free;
+		twl->chip.npwm = 1;
+	}
+
+	twl->chip.dev = &pdev->dev;
+	twl->chip.ops = &twl_pwmled_ops;
+	twl->chip.base = -1;
+
+	ret = pwmchip_add(&twl->chip);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, twl);
+
+	return 0;
+}
+
+static int twl_pwmled_remove(struct platform_device *pdev)
+{
+	struct twl_pwmled_chip *twl = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&twl->chip);
+}
+
+static struct of_device_id twl_pwmled_of_match[] = {
+	{ .compatible = "ti,twl4030-pwmled" },
+	{ .compatible = "ti,twl6030-pwmled" },
+};
+
+MODULE_DEVICE_TABLE(of, twl_pwmled_of_match);
+
+static struct platform_driver twl_pwmled_driver = {
+	.driver = {
+		.name = "twl-pwmled",
+		.of_match_table = of_match_ptr(twl_pwmled_of_match),
+	},
+	.probe = twl_pwmled_probe,
+	.remove = __devexit_p(twl_pwmled_remove),
+};
+module_platform_driver(twl_pwmled_driver);
+
+MODULE_AUTHOR("Peter Ujfalusi <peter.ujfalusi@ti.com>");
+MODULE_DESCRIPTION("PWM driver for TWL4030 and TWL6030 LED outputs");
+MODULE_ALIAS("platform:twl-pwm");
+MODULE_LICENSE("GPL");
-- 
1.8.0

Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs

[Peter Ujfalusi]

On 11/20/2012 10:56 AM, Peter Ujfalusi wrote:
> The driver supports the following LED outputs as generic PWM driver:
> TWL4030 LEDA and LEDB (PWMA and PWMB)
> TWL6030 Charging indicator LED (PWM LED)
> 
> On TWL6030 when the PWM requested LED is configured to be controlled by SW.
> In this case the user can enable/disable and set the duty period freely.
> When the PWM has been freed, the LED driver is put back to HW control.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/pwm/Kconfig       |  10 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-twl-led.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 314 insertions(+)
>  create mode 100644 drivers/pwm/pwm-twl-led.c

...

> +static int twl4030_pwmled_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      int duty_ns, int period_ns)
> +{
> +	int duty_cycle = DIV_ROUND_UP(duty_ns * TWL4030_LED_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,
> +	 * 125 - > off cycle 127,
> +	 * 126 - > off cycle 1

Oh, I missed to save the updated comment before I commited the change.
Can I just send and update patch instead of the whole series again?

> +	 * When on cycle == off cycle the PWM will be always on
> +	 */


-- 
Péter

Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs

[Thierry Reding]

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

On Tue, Nov 20, 2012 at 12:54:11PM +0100, Peter Ujfalusi wrote:
> On 11/20/2012 10:56 AM, Peter Ujfalusi wrote:
> > The driver supports the following LED outputs as generic PWM driver:
> > TWL4030 LEDA and LEDB (PWMA and PWMB)
> > TWL6030 Charging indicator LED (PWM LED)
> > 
> > On TWL6030 when the PWM requested LED is configured to be controlled by SW.
> > In this case the user can enable/disable and set the duty period freely.
> > When the PWM has been freed, the LED driver is put back to HW control.
> > 
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > ---
> >  drivers/pwm/Kconfig       |  10 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-twl-led.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 314 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-twl-led.c
> 
> ...
> 
> > +static int twl4030_pwmled_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			      int duty_ns, int period_ns)
> > +{
> > +	int duty_cycle = DIV_ROUND_UP(duty_ns * TWL4030_LED_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,
> > +	 * 125 - > off cycle 127,
> > +	 * 126 - > off cycle 1
> 
> Oh, I missed to save the updated comment before I commited the change.
> Can I just send and update patch instead of the whole series again?

Sure.

Sorry for taking so long to review this. I'm rather busy with other
things but I'll try to find some time to review properly tonight or
tomorrow.

Thierry

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

Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs

[Peter Ujfalusi]

Hi Thierry,

On 11/20/2012 12:58 PM, Thierry Reding wrote:
>> Oh, I missed to save the updated comment before I commited the change.
>> Can I just send and update patch instead of the whole series again?
> 
> Sure.
> 
> Sorry for taking so long to review this. I'm rather busy with other
> things but I'll try to find some time to review properly tonight or
> tomorrow.

I have sent the update patch for the series:
https://lkml.org/lkml/2012/11/20/268

I know you are busy, but would you be able to take a look at the series?

Thank you,
Péter

Re: [PATCH v3 2/3] pwm: New driver to support PWMs on TWL4030/6030 series of PMICs

[Thierry Reding]

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

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
> +		/* PWM 0 */
> +		mask = TWL4030_GPIO6_PWM0_MUTE_MASK;

I'm not sure how useful these comments are. You have both an explicit
check for pwm->hwpwm to make it obvious that it isn't 0 and the mask
macros contain the PWM0 and PWM1 substrings respectively.

You could make it even more explicit perhaps by turning the check into:

	if (pwm->hwpwm == 1)

But either way I think you can drop the /* PWM 1 */ and /* PWM 0 */
comments.

> +
> +	/* Restore the MUX configuration for the PWM */
> +	val &= ~mask;
> +	val |= (twl->twl4030_pwm_mux & mask);
> +
> +	ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
> +	if (ret < 0)
> +		dev_err(chip->dev, "%s: Failed to free PWM\n", pwm->label);
> +}
> +
> +static int twl6030_pwm_enable(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;
> +
> +	val = twl->twl6030_toggle3;
> +	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
> +	val &= ~TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXR);
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to enable PWM\n", pwm->label);
> +		return ret;
> +	}
> +
> +	twl->twl6030_toggle3 = val;
> +
> +	return 0;
> +}
> +
> +static void twl6030_pwm_disable(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;
> +
> +	val = twl->twl6030_toggle3;
> +	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXR);
> +	val &= ~TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to read TOGGLE3\n", pwm->label);
> +		return;
> +	}
> +
> +	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
> +		return;
> +	}
> +
> +	twl->twl6030_toggle3 = val;
> +}
> +
> +static struct pwm_ops twl_pwm_ops = {
> +	.config = twl_pwm_config,
> +};

It might actually be worth to split this into two pwm_ops structures,
one for 4030 and one for 6030. In that case you would still be able to
make them const, which probably outweighs the space savings here.

Actually, I think this is even potentially buggy since you may have more
than one instance of this driver. For instance if you have a TWL6030 and
a TWL4030 in a single system this will break horribly since you'll over-
write the pwm_ops of one of them.

> +	if (twl_class_is_4030()) {
> +		twl_pwm_ops.enable = twl4030_pwm_enable;
> +		twl_pwm_ops.disable = twl4030_pwm_disable;
> +		twl_pwm_ops.request = twl4030_pwm_request;
> +		twl_pwm_ops.free = twl4030_pwm_free;

This would become:

		twl->chip.ops = &twl4030_pwm_ops;

> +	} else {
> +		twl_pwm_ops.enable = twl6030_pwm_enable;
> +		twl_pwm_ops.disable = twl6030_pwm_disable;

and:
		twl->chip.ops = &twl6030_pwm_ops;

> +static struct of_device_id twl_pwm_of_match[] = {
> +	{ .compatible = "ti,twl4030-pwm" },
> +	{ .compatible = "ti,twl6030-pwm" },
> +};
> +
> +MODULE_DEVICE_TABLE(of, twl_pwm_of_match);

Nit: I think the blank line between "};" and "MODULE_DEVICE_TABLE(...)"
can go away. And you might want to protect this with an "#ifdef
CONFIG_OF" since you don't depend on OF.

Thierry

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

Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs

[Thierry Reding]

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

On Tue, Nov 20, 2012 at 10:56:22AM +0100, Peter Ujfalusi wrote:
> The driver supports the following LED outputs as generic PWM driver:
> TWL4030 LEDA and LEDB (PWMA and PWMB)
> TWL6030 Charging indicator LED (PWM LED)
> 
> On TWL6030 when the PWM requested LED is configured to be controlled by SW.
> In this case the user can enable/disable and set the duty period freely.
> When the PWM has been freed, the LED driver is put back to HW control.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/pwm/Kconfig       |  10 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-twl-led.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 314 insertions(+)
>  create mode 100644 drivers/pwm/pwm-twl-led.c

Doesn't this belong in the drivers/leds subsystem? Besides that, the
same comments as for the previous patch apply. One additional note
below.

> +static struct platform_driver twl_pwmled_driver = {
> +	.driver = {
> +		.name = "twl-pwmled",
> +		.of_match_table = of_match_ptr(twl_pwmled_of_match),
> +	},
> +	.probe = twl_pwmled_probe,
> +	.remove = __devexit_p(twl_pwmled_remove),

You didn't annotate twl_pwmled_remove() with __devexit, so __devexit_p
isn't needed here either.

Thierry

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

Re: [PATCH v3 1/3] pwm: Remove pwm-twl6030 driver

[Thierry Reding]

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

On Tue, Nov 20, 2012 at 10:56:20AM +0100, Peter Ujfalusi wrote:
> This driver only supported the Charging indicator LED.
> New set of drivers going to provide support for both PWMs and LEDs for twl4030
> and twl6030 series of PMICs.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/pwm/Kconfig       |   9 ---
>  drivers/pwm/Makefile      |   1 -
>  drivers/pwm/pwm-twl6030.c | 184 ----------------------------------------------
>  3 files changed, 194 deletions(-)
>  delete mode 100644 drivers/pwm/pwm-twl6030.c

I think I'd rather see this patch as the last in the series to make sure
that no functionality is dropped across the series.

Thierry

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

Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs

[Peter Ujfalusi]

On 11/23/2012 04:04 PM, Thierry Reding wrote:
> On Tue, Nov 20, 2012 at 10:56:22AM +0100, Peter Ujfalusi wrote:
>> The driver supports the following LED outputs as generic PWM driver:
>> TWL4030 LEDA and LEDB (PWMA and PWMB)
>> TWL6030 Charging indicator LED (PWM LED)
>>
>> On TWL6030 when the PWM requested LED is configured to be controlled by SW.
>> In this case the user can enable/disable and set the duty period freely.
>> When the PWM has been freed, the LED driver is put back to HW control.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/pwm/Kconfig       |  10 ++
>>  drivers/pwm/Makefile      |   1 +
>>  drivers/pwm/pwm-twl-led.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 314 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-twl-led.c
> 
> Doesn't this belong in the drivers/leds subsystem? Besides that, the
> same comments as for the previous patch apply. One additional note
> below.

The PINs itself are called as LED but they are PWMs at the end. If we
represent them as PWMs they can be used for different purposes which is going
to be needed for example in BeagleBoard, where the LEDA (PWMA) is used as a
GPO to enable/disable the USB host power.
Also the removed 'twl6030-pwm' driver was only controlled the LED part of twl6030.
With this series I enable the use of the PWMs and the PWMs behind of the LED
functions to give us flexibility on how we are using them.

> 
>> +static struct platform_driver twl_pwmled_driver = {
>> +	.driver = {
>> +		.name = "twl-pwmled",
>> +		.of_match_table = of_match_ptr(twl_pwmled_of_match),
>> +	},
>> +	.probe = twl_pwmled_probe,
>> +	.remove = __devexit_p(twl_pwmled_remove),
> 
> You didn't annotate twl_pwmled_remove() with __devexit, so __devexit_p
> isn't needed here either.

Oh yes, I have also received patches from a series which removes the
_devexit_p() from the kernel.
But should the __devexit need to be added to the remove function?

-- 
Péter

Re: [PATCH v3 1/3] pwm: Remove pwm-twl6030 driver

[Peter Ujfalusi]

On 11/23/2012 04:05 PM, Thierry Reding wrote:
> On Tue, Nov 20, 2012 at 10:56:20AM +0100, Peter Ujfalusi wrote:
>> This driver only supported the Charging indicator LED.
>> New set of drivers going to provide support for both PWMs and LEDs for twl4030
>> and twl6030 series of PMICs.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/pwm/Kconfig       |   9 ---
>>  drivers/pwm/Makefile      |   1 -
>>  drivers/pwm/pwm-twl6030.c | 184 ----------------------------------------------
>>  3 files changed, 194 deletions(-)
>>  delete mode 100644 drivers/pwm/pwm-twl6030.c
> 
> I think I'd rather see this patch as the last in the series to make sure
> that no functionality is dropped across the series.

Sure, I will move it as a last patch.

-- 
Péter

Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs

[Thierry Reding]

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

On Mon, Nov 26, 2012 at 09:30:03AM +0100, Peter Ujfalusi wrote:
> On 11/23/2012 04:04 PM, Thierry Reding wrote:
> > On Tue, Nov 20, 2012 at 10:56:22AM +0100, Peter Ujfalusi wrote:
> >> The driver supports the following LED outputs as generic PWM driver:
> >> TWL4030 LEDA and LEDB (PWMA and PWMB)
> >> TWL6030 Charging indicator LED (PWM LED)
> >>
> >> On TWL6030 when the PWM requested LED is configured to be controlled by SW.
> >> In this case the user can enable/disable and set the duty period freely.
> >> When the PWM has been freed, the LED driver is put back to HW control.
> >>
> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> >> ---
> >>  drivers/pwm/Kconfig       |  10 ++
> >>  drivers/pwm/Makefile      |   1 +
> >>  drivers/pwm/pwm-twl-led.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 314 insertions(+)
> >>  create mode 100644 drivers/pwm/pwm-twl-led.c
> > 
> > Doesn't this belong in the drivers/leds subsystem? Besides that, the
> > same comments as for the previous patch apply. One additional note
> > below.
> 
> The PINs itself are called as LED but they are PWMs at the end. If we
> represent them as PWMs they can be used for different purposes which is going
> to be needed for example in BeagleBoard, where the LEDA (PWMA) is used as a
> GPO to enable/disable the USB host power.

Heh, that's an interesting use-case for a PWM. =)

> Also the removed 'twl6030-pwm' driver was only controlled the LED part of twl6030.
> With this series I enable the use of the PWMs and the PWMs behind of the LED
> functions to give us flexibility on how we are using them.

Alright, we can keep it in the PWM subsystem then.

> >> +static struct platform_driver twl_pwmled_driver = {
> >> +	.driver = {
> >> +		.name = "twl-pwmled",
> >> +		.of_match_table = of_match_ptr(twl_pwmled_of_match),
> >> +	},
> >> +	.probe = twl_pwmled_probe,
> >> +	.remove = __devexit_p(twl_pwmled_remove),
> > 
> > You didn't annotate twl_pwmled_remove() with __devexit, so __devexit_p
> > isn't needed here either.
> 
> Oh yes, I have also received patches from a series which removes the
> _devexit_p() from the kernel.
> But should the __devexit need to be added to the remove function?

__devexit_p without a corresponding __devexit doesn't make sense. But as
all of __devinit, __devexit and __devexit_p will be removed sooner or
later, new code just shouldn't bother adding it. In this case, just drop
__devexit_p.

Thierry

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

Re: [PATCH v3 2/3] pwm: New driver to support PWMs on TWL4030/6030 series of PMICs

[Peter Ujfalusi]

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, 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
>> +		/* PWM 0 */
>> +		mask = TWL4030_GPIO6_PWM0_MUTE_MASK;
> 
> I'm not sure how useful these comments are. You have both an explicit
> check for pwm->hwpwm to make it obvious that it isn't 0 and the mask
> macros contain the PWM0 and PWM1 substrings respectively.
> 
> You could make it even more explicit perhaps by turning the check into:
> 
> 	if (pwm->hwpwm == 1)
> 
> But either way I think you can drop the /* PWM 1 */ and /* PWM 0 */
> comments.

OK will drop the comment.

> 
>> +
>> +	/* Restore the MUX configuration for the PWM */
>> +	val &= ~mask;
>> +	val |= (twl->twl4030_pwm_mux & mask);
>> +
>> +	ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
>> +	if (ret < 0)
>> +		dev_err(chip->dev, "%s: Failed to free PWM\n", pwm->label);
>> +}
>> +
>> +static int twl6030_pwm_enable(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;
>> +
>> +	val = twl->twl6030_toggle3;
>> +	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
>> +	val &= ~TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXR);
>> +
>> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
>> +	if (ret < 0) {
>> +		dev_err(chip->dev, "%s: Failed to enable PWM\n", pwm->label);
>> +		return ret;
>> +	}
>> +
>> +	twl->twl6030_toggle3 = val;
>> +
>> +	return 0;
>> +}
>> +
>> +static void twl6030_pwm_disable(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;
>> +
>> +	val = twl->twl6030_toggle3;
>> +	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXR);
>> +	val &= ~TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
>> +
>> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
>> +	if (ret < 0) {
>> +		dev_err(chip->dev, "%s: Failed to read TOGGLE3\n", pwm->label);
>> +		return;
>> +	}
>> +
>> +	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
>> +
>> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
>> +	if (ret < 0) {
>> +		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
>> +		return;
>> +	}
>> +
>> +	twl->twl6030_toggle3 = val;
>> +}
>> +
>> +static struct pwm_ops twl_pwm_ops = {
>> +	.config = twl_pwm_config,
>> +};
> 
> It might actually be worth to split this into two pwm_ops structures,
> one for 4030 and one for 6030. In that case you would still be able to
> make them const, which probably outweighs the space savings here.
> 
> Actually, I think this is even potentially buggy since you may have more
> than one instance of this driver. For instance if you have a TWL6030 and
> a TWL4030 in a single system this will break horribly since you'll over-
> write the pwm_ops of one of them.

True, I will split them. However the twl4030 and twl6030 can not be on the
same system. twl4030 is for OMAP3 systems and twl6030 is used only on OMAP4
based devices. But you are right.

> 
>> +	if (twl_class_is_4030()) {
>> +		twl_pwm_ops.enable = twl4030_pwm_enable;
>> +		twl_pwm_ops.disable = twl4030_pwm_disable;
>> +		twl_pwm_ops.request = twl4030_pwm_request;
>> +		twl_pwm_ops.free = twl4030_pwm_free;
> 
> This would become:
> 
> 		twl->chip.ops = &twl4030_pwm_ops;
> 
>> +	} else {
>> +		twl_pwm_ops.enable = twl6030_pwm_enable;
>> +		twl_pwm_ops.disable = twl6030_pwm_disable;
> 
> and:
> 		twl->chip.ops = &twl6030_pwm_ops;
> 
>> +static struct of_device_id twl_pwm_of_match[] = {
>> +	{ .compatible = "ti,twl4030-pwm" },
>> +	{ .compatible = "ti,twl6030-pwm" },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, twl_pwm_of_match);
> 
> Nit: I think the blank line between "};" and "MODULE_DEVICE_TABLE(...)"
> can go away.

True.

> And you might want to protect this with an "#ifdef
> CONFIG_OF" since you don't depend on OF.

The of.h already takes care of this when we do not have CONFIG_OF.

Thank you,
Péter

Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs

[Peter Ujfalusi]

On 11/26/2012 10:17 AM, Thierry Reding wrote:
>>> Doesn't this belong in the drivers/leds subsystem? Besides that, the
>>> same comments as for the previous patch apply. One additional note
>>> below.
>>
>> The PINs itself are called as LED but they are PWMs at the end. If we
>> represent them as PWMs they can be used for different purposes which is going
>> to be needed for example in BeagleBoard, where the LEDA (PWMA) is used as a
>> GPO to enable/disable the USB host power.
> 
> Heh, that's an interesting use-case for a PWM. =)

You should have seen the expression on my face when I saw this on the
schematics ;)

>> Also the removed 'twl6030-pwm' driver was only controlled the LED part of twl6030.
>> With this series I enable the use of the PWMs and the PWMs behind of the LED
>> functions to give us flexibility on how we are using them.
> 
> Alright, we can keep it in the PWM subsystem then.

Thank you.

>>>> +static struct platform_driver twl_pwmled_driver = {
>>>> +	.driver = {
>>>> +		.name = "twl-pwmled",
>>>> +		.of_match_table = of_match_ptr(twl_pwmled_of_match),
>>>> +	},
>>>> +	.probe = twl_pwmled_probe,
>>>> +	.remove = __devexit_p(twl_pwmled_remove),
>>>
>>> You didn't annotate twl_pwmled_remove() with __devexit, so __devexit_p
>>> isn't needed here either.
>>
>> Oh yes, I have also received patches from a series which removes the
>> _devexit_p() from the kernel.
>> But should the __devexit need to be added to the remove function?
> 
> __devexit_p without a corresponding __devexit doesn't make sense. But as
> all of __devinit, __devexit and __devexit_p will be removed sooner or
> later, new code just shouldn't bother adding it. In this case, just drop
> __devexit_p.

I'll get rid of them.

Thank you,
Péter