[PATCH 2/2 v2] pwm: add a driver for the STMPE PWM

[PATCH 2/2 v2] pwm: add a driver for the STMPE PWM

[Linus Walleij]

This adds a driver for the STMPE 24xx series of multi-purpose
I2C expanders. (I think STMPE means ST Microelectronics
Multi-Purpose Expander.) This PWM was designed in accordance with
Nokia specifications and is kind of weird and usually just
switched between max and zero dutycycle. However it is indeed
a PWM so it needs to live in the PWM subsystem.

This PWM is mostly used for white LED backlight.

Cc: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Split off the MFD hunk from the patch: no need to worry about
  that, it can be merged orthogonally to the MFD tree.
---
 drivers/pwm/Kconfig     |   7 ++
 drivers/pwm/Makefile    |   1 +
 drivers/pwm/pwm-stmpe.c | 297 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 305 insertions(+)
 create mode 100644 drivers/pwm/pwm-stmpe.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8cf0dae78555..89665c6d6a4d 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -362,6 +362,13 @@ config PWM_STI
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sti.
 
+config PWM_STMPE
+	bool "STMPE expander PWM export"
+	depends on MFD_STMPE
+	help
+	  This enables support for the PWMs found in the STMPE I/O
+	  expanders.
+
 config PWM_SUN4I
 	tristate "Allwinner PWM support"
 	depends on ARCH_SUNXI || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index dd35bc121a18..790353b7454e 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
+obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
 obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
diff --git a/drivers/pwm/pwm-stmpe.c b/drivers/pwm/pwm-stmpe.c
new file mode 100644
index 000000000000..3da117a92263
--- /dev/null
+++ b/drivers/pwm/pwm-stmpe.c
@@ -0,0 +1,297 @@
+/*
+ * Copyright (C) 2016 Linaro Ltd.
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * 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.
+ *
+ */
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/pwm.h>
+#include <linux/of.h>
+#include <linux/mfd/stmpe.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+
+#define STMPE24XX_PWMCS		0x30
+#define PWMCS_EN_PWM0		BIT(0)
+#define PWMCS_EN_PWM1		BIT(1)
+#define PWMCS_EN_PWM2		BIT(2)
+#define STMPE24XX_PWMIC0	0x38
+#define STMPE24XX_PWMIC1	0x39
+#define STMPE24XX_PWMIC2	0x3a
+#define STMPE_PWM_24XX_PINBASE	21
+
+struct stmpe_pwm {
+	struct stmpe *stmpe;
+	struct pwm_chip chip;
+	u8 lastduty;
+	bool enabled;
+};
+
+static inline struct stmpe_pwm *to_stmpe_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct stmpe_pwm, chip);
+}
+
+static int stmpe_24xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
+	int ret;
+	u8 val;
+
+	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
+	if (ret < 0) {
+		dev_err(chip->dev, "error reading PWM%d control\n",
+			pwm->hwpwm);
+		return ret;
+	}
+	val = (u8)ret;
+	val |= BIT(pwm->hwpwm);
+	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, val);
+	if (ret) {
+		dev_err(chip->dev, "error writing PWM%d control\n",
+			pwm->hwpwm);
+		return ret;
+	}
+	stmpe_pwm->enabled = true;
+
+	return 0;
+}
+
+static void stmpe_24xx_pwm_disable(struct pwm_chip *chip,
+				struct pwm_device *pwm)
+{
+	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
+	int ret;
+	u8 val;
+
+	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
+	if (ret < 0) {
+		dev_err(chip->dev, "error reading PWM%d control\n",
+			pwm->hwpwm);
+		return;
+	}
+	val = (u8)ret;
+	val &= ~BIT(pwm->hwpwm);
+	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, val);
+	if (ret) {
+		dev_err(chip->dev, "error writing PWM%d control\n",
+			pwm->hwpwm);
+		return;
+	}
+	stmpe_pwm->enabled = false;
+}
+
+static int stmpe_24xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+				int duty_ns, int period_ns)
+{
+	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
+	u8 reg;
+	int ret;
+	int i;
+	unsigned int pin;
+	u16 program[3] = {
+		0x007F, /* SMAX = logic low out */
+		0x0000, /* GTS */
+		0x0000, /* GTS */
+	};
+
+	/* This preserves enablement state across reconfig */
+	bool enabled = stmpe_pwm->enabled;
+
+	/* Make sure we are disabled */
+	if (enabled)
+		stmpe_24xx_pwm_disable(chip, pwm);
+
+	/* Connect the PWM to the pin */
+	pin = pwm->hwpwm;
+	/* On STMPE2401 and 2403 pins 21,22,23 are used */
+	if (stmpe_pwm->stmpe->partnum == STMPE2401 ||
+		stmpe_pwm->stmpe->partnum == STMPE2403)
+		pin += STMPE_PWM_24XX_PINBASE;
+	ret = stmpe_set_altfunc(stmpe_pwm->stmpe,
+				BIT(pin),
+				STMPE_BLOCK_PWM);
+	if (ret) {
+		dev_err(chip->dev, "unable to connect PWM%d to pin\n",
+			pwm->hwpwm);
+		return ret;
+	}
+
+	/* STMPE24XX */
+	if (pwm->hwpwm == 0)
+		reg = STMPE24XX_PWMIC0;
+	else if (pwm->hwpwm == 1)
+		reg = STMPE24XX_PWMIC1;
+	else if (pwm->hwpwm == 2)
+		reg = STMPE24XX_PWMIC1;
+	else
+		/* Should not happen as npwm is 3 */
+		return -ENODEV;
+
+	dev_dbg(chip->dev, "PWM%d: config duty %d ns, period %d ns\n",
+		pwm->hwpwm, duty_ns, period_ns);
+	if (duty_ns == 0) {
+		if (stmpe_pwm->stmpe->partnum == STMPE2401)
+			program[0] = 0x007F; /* SMAX = off all the time */
+		if (stmpe_pwm->stmpe->partnum == STMPE2403)
+			program[0] = BIT(14) | 0xFF; /* LOAD 0xFF */
+		stmpe_pwm->lastduty = 0x00;
+	} else if (duty_ns == period_ns) {
+		if (stmpe_pwm->stmpe->partnum == STMPE2401)
+			program[0] = 0x00FF; /* SMIN = on all the time */
+		if (stmpe_pwm->stmpe->partnum == STMPE2403)
+			program[0] = BIT(14); /* LOAD 0x00 */
+		stmpe_pwm->lastduty = 0xFF;
+	} else {
+		unsigned long cyc;
+		u8 val;
+
+		/*
+		 * Counter goes from 0x00 to 0xFF repeatedly at 32768 Hz,
+		 * (means a period of 30517 ns)
+		 * then this is compared to the counter from the ramp,
+		 * if this is >= pwm counter the output is high.
+		 * With LOAD we can define how much of the cycle it is on.
+		 *
+		 * Prescale = 0 -> 2 kHz -> T = 1/f = 488281,25 ns
+		 */
+		/* Scale to 0..FF */
+		cyc = duty_ns * 256;
+		cyc = DIV_ROUND_CLOSEST(cyc, period_ns);
+		val = cyc;
+
+		if (val == stmpe_pwm->lastduty) {
+			/* Do nothing */
+		} else if (stmpe_pwm->stmpe->partnum == STMPE2403) {
+			/* STMPE2403 can simply set the right PWM value */
+			program[0] = BIT(14) | val;
+			program[1] = 0x0000;
+		} else if (stmpe_pwm->stmpe->partnum == STMPE2401) {
+			/* STMPE2401 need a complex program */
+			u16 incdec = 0x0000;
+
+			if (stmpe_pwm->lastduty < val)
+				/* Count up */
+				incdec = (val - stmpe_pwm->lastduty);
+			else
+				/* Count down */
+				incdec = BIT(7) | (stmpe_pwm->lastduty - val);
+			/* Step to desired value, smoothly */
+			program[0] = BIT(14) | BIT(8) | incdec;
+			/* Loop eternally */
+			program[1] = BIT(15) | BIT(13);
+		}
+
+		dev_dbg(chip->dev, "PWM%d: val = %02x, lastduty = %02x, "
+			"program=%04x,%04x,%04x\n", pwm->hwpwm, val,
+			stmpe_pwm->lastduty, program[0], program[1],
+			program[2]);
+		stmpe_pwm->lastduty = val;
+	}
+
+	/*
+	 * We can write programs of up to 64 16-bit words into this
+	 * channel.
+	 */
+	for (i = 0; i < ARRAY_SIZE(program); i++) {
+		u8 val;
+
+		val = (program[i] >> 8) & 0xFF;
+		ret = stmpe_reg_write(stmpe_pwm->stmpe, reg, val);
+		if (ret) {
+			dev_err(chip->dev, "error writing reg\n");
+			return ret;
+		}
+		val = program[i] & 0xFF;
+		ret = stmpe_reg_write(stmpe_pwm->stmpe, reg, val);
+		if (ret) {
+			dev_err(chip->dev, "error writing reg\n");
+			return ret;
+		}
+	}
+	/* If we were enabled, re-enable this PWM */
+	if (enabled)
+		stmpe_24xx_pwm_enable(chip, pwm);
+
+	/* Sleep for 200ms so we're sure it will take effect */
+	msleep(200);
+
+	dev_dbg(chip->dev, "programmed PWM%d, %d bytes\n",
+		pwm->hwpwm, i);
+	return 0;
+}
+
+static const struct pwm_ops stmpe_24xx_pwm_ops = {
+	.config = stmpe_24xx_pwm_config,
+	.enable = stmpe_24xx_pwm_enable,
+	.disable = stmpe_24xx_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int stmpe_pwm_probe(struct platform_device *pdev)
+{
+	struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
+	struct stmpe_pwm *pwm;
+	int ret;
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->stmpe = stmpe;
+	pwm->chip.dev = &pdev->dev;
+	pwm->chip.base = pdev->id;
+	if (stmpe->partnum == STMPE2401 || stmpe->partnum == STMPE2403) {
+		pwm->chip.ops = &stmpe_24xx_pwm_ops;
+		pwm->chip.npwm = 3;
+	} else if (stmpe->partnum == STMPE1601) {
+		dev_err(&pdev->dev, "STMPE1601 PWM not yet supported\n");
+		return -EINVAL;
+	} else {
+		dev_err(&pdev->dev, "Unsupported STMPE PWM\n");
+		return -EINVAL;
+	}
+
+	ret = stmpe_enable(stmpe, STMPE_BLOCK_PWM);
+	if (ret)
+		return ret;
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret)
+		return ret;
+
+	dev_info(&pdev->dev, "STMPE PWM registered\n");
+	platform_set_drvdata(pdev, pwm);
+
+	return 0;
+}
+
+static int stmpe_pwm_remove(struct platform_device *pdev)
+{
+	struct stmpe_pwm *pwm = platform_get_drvdata(pdev);
+	struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
+
+	stmpe_disable(stmpe, STMPE_BLOCK_PWM);
+	return pwmchip_remove(&pwm->chip);
+}
+
+static struct platform_driver stmpe_pwm_driver = {
+	.driver = {
+		.name = "stmpe-pwm",
+	},
+	.probe = stmpe_pwm_probe,
+	.remove = stmpe_pwm_remove,
+};
+module_platform_driver(stmpe_pwm_driver);
+
+MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
+MODULE_DESCRIPTION("STMPE Pulse Width Modulation Driver");
+MODULE_ALIAS("platform:pwm-stmpe");
+MODULE_LICENSE("GPL v2");
-- 
2.4.3

Re: [PATCH 2/2 v2] pwm: add a driver for the STMPE PWM

[Thierry Reding]

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

On Sun, Feb 14, 2016 at 02:08:45PM +0100, Linus Walleij wrote:
> This adds a driver for the STMPE 24xx series of multi-purpose
> I2C expanders. (I think STMPE means ST Microelectronics
> Multi-Purpose Expander.) This PWM was designed in accordance with
> Nokia specifications and is kind of weird and usually just
> switched between max and zero dutycycle. However it is indeed
> a PWM so it needs to live in the PWM subsystem.
> 
> This PWM is mostly used for white LED backlight.
> 
> Cc: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Split off the MFD hunk from the patch: no need to worry about
>   that, it can be merged orthogonally to the MFD tree.
> ---
>  drivers/pwm/Kconfig     |   7 ++
>  drivers/pwm/Makefile    |   1 +
>  drivers/pwm/pwm-stmpe.c | 297 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 305 insertions(+)
>  create mode 100644 drivers/pwm/pwm-stmpe.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8cf0dae78555..89665c6d6a4d 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -362,6 +362,13 @@ config PWM_STI
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-sti.
>  
> +config PWM_STMPE
> +	bool "STMPE expander PWM export"

tristate maybe? If not I suspect that we'll get a patch to this driver
shortly that will convert the module_platform_driver() to a
builtin_platform_driver().

> +	depends on MFD_STMPE
> +	help
> +	  This enables support for the PWMs found in the STMPE I/O
> +	  expanders.
> +
>  config PWM_SUN4I
>  	tristate "Allwinner PWM support"
>  	depends on ARCH_SUNXI || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index dd35bc121a18..790353b7454e 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
> +obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
> diff --git a/drivers/pwm/pwm-stmpe.c b/drivers/pwm/pwm-stmpe.c
[...]
> +struct stmpe_pwm {
> +	struct stmpe *stmpe;
> +	struct pwm_chip chip;
> +	u8 lastduty;
> +	bool enabled;

Can you not use the pwm_is_enabled() to track this?

> +static int stmpe_24xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
> +	int ret;
> +	u8 val;
> +
> +	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "error reading PWM%d control\n",
> +			pwm->hwpwm);
> +		return ret;
> +	}
> +	val = (u8)ret;
> +	val |= BIT(pwm->hwpwm);
> +	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, val);

Do you even need the cast to u8 here? Seems to me like it should all
work properly if you keep using ret (possibly renamed) here. Either way
is fine with me, though.

Perhaps add a couple more blank lines to make this easier on the eye. I
like to have blocks separated by blank lines, so a blank line after '}'
and before 'ret = ...' would be nice.

> +	if (ret) {
> +		dev_err(chip->dev, "error writing PWM%d control\n",
> +			pwm->hwpwm);
> +		return ret;
> +	}
> +	stmpe_pwm->enabled = true;
> +
> +	return 0;
> +}
> +
> +static void stmpe_24xx_pwm_disable(struct pwm_chip *chip,
> +				struct pwm_device *pwm)
> +{
> +	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
> +	int ret;
> +	u8 val;
> +
> +	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "error reading PWM%d control\n",
> +			pwm->hwpwm);
> +		return;
> +	}
> +	val = (u8)ret;
> +	val &= ~BIT(pwm->hwpwm);
> +	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, val);
> +	if (ret) {
> +		dev_err(chip->dev, "error writing PWM%d control\n",
> +			pwm->hwpwm);
> +		return;
> +	}
> +	stmpe_pwm->enabled = false;
> +}

Same here, re: the blank lines.

> +
> +static int stmpe_24xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +				int duty_ns, int period_ns)
> +{
> +	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
> +	u8 reg;
> +	int ret;
> +	int i;

i can be unsigned int, and then merged into the below line that declares
pin.

> +	unsigned int pin;
> +	u16 program[3] = {
> +		0x007F, /* SMAX = logic low out */
> +		0x0000, /* GTS */
> +		0x0000, /* GTS */
> +	};
> +
> +	/* This preserves enablement state across reconfig */
> +	bool enabled = stmpe_pwm->enabled;
> +
> +	/* Make sure we are disabled */
> +	if (enabled)
> +		stmpe_24xx_pwm_disable(chip, pwm);

pwm_is_enabled() should do the trick here, shouldn't it?

> +	/* Connect the PWM to the pin */
> +	pin = pwm->hwpwm;
> +	/* On STMPE2401 and 2403 pins 21,22,23 are used */
> +	if (stmpe_pwm->stmpe->partnum == STMPE2401 ||
> +		stmpe_pwm->stmpe->partnum == STMPE2403)
> +		pin += STMPE_PWM_24XX_PINBASE;

This might be better as SoC specific data associated with this device
via of_device_id->data. That way you can properly parameterize and it
should be more forward-portable.

> +	ret = stmpe_set_altfunc(stmpe_pwm->stmpe,
> +				BIT(pin),
> +				STMPE_BLOCK_PWM);
> +	if (ret) {
> +		dev_err(chip->dev, "unable to connect PWM%d to pin\n",
> +			pwm->hwpwm);
> +		return ret;
> +	}
> +
> +	/* STMPE24XX */
> +	if (pwm->hwpwm == 0)
> +		reg = STMPE24XX_PWMIC0;
> +	else if (pwm->hwpwm == 1)
> +		reg = STMPE24XX_PWMIC1;
> +	else if (pwm->hwpwm == 2)
> +		reg = STMPE24XX_PWMIC1;
> +	else
> +		/* Should not happen as npwm is 3 */
> +		return -ENODEV;

It's dead code, you might as well remove it.

> +
> +	dev_dbg(chip->dev, "PWM%d: config duty %d ns, period %d ns\n",
> +		pwm->hwpwm, duty_ns, period_ns);
> +	if (duty_ns == 0) {
> +		if (stmpe_pwm->stmpe->partnum == STMPE2401)
> +			program[0] = 0x007F; /* SMAX = off all the time */
> +		if (stmpe_pwm->stmpe->partnum == STMPE2403)
> +			program[0] = BIT(14) | 0xFF; /* LOAD 0xFF */
> +		stmpe_pwm->lastduty = 0x00;
> +	} else if (duty_ns == period_ns) {
> +		if (stmpe_pwm->stmpe->partnum == STMPE2401)
> +			program[0] = 0x00FF; /* SMIN = on all the time */
> +		if (stmpe_pwm->stmpe->partnum == STMPE2403)
> +			program[0] = BIT(14); /* LOAD 0x00 */
> +		stmpe_pwm->lastduty = 0xFF;
> +	} else {

Again, it might be easier to parameterize these, though admittedly this
would be somewhat more complicated than for the pin base. I'm fine with
constructs such as this for now, but if the list of part numbers starts
to increase, I'll likely request this to be changed to something easier
to maintain.

Also there are a bunch of BIT(X) in the above (and below). Can we get
some symbolic names for those, please?

> +		unsigned long cyc;
> +		u8 val;
> +
> +		/*
> +		 * Counter goes from 0x00 to 0xFF repeatedly at 32768 Hz,
> +		 * (means a period of 30517 ns)
> +		 * then this is compared to the counter from the ramp,
> +		 * if this is >= pwm counter the output is high.
> +		 * With LOAD we can define how much of the cycle it is on.
> +		 *
> +		 * Prescale = 0 -> 2 kHz -> T = 1/f = 488281,25 ns
> +		 */
> +		/* Scale to 0..FF */
> +		cyc = duty_ns * 256;
> +		cyc = DIV_ROUND_CLOSEST(cyc, period_ns);
> +		val = cyc;
> +
> +		if (val == stmpe_pwm->lastduty) {
> +			/* Do nothing */

This fall-through will cause the program to be written to the channel,
is that really what you want? This looks slightly odd, perhaps inverting
the condition and putting the code below into its block would make this
more explicit. Maybe a "goto write_program;" would be equally explicit.

> +		} else if (stmpe_pwm->stmpe->partnum == STMPE2403) {
> +			/* STMPE2403 can simply set the right PWM value */
> +			program[0] = BIT(14) | val;
> +			program[1] = 0x0000;
> +		} else if (stmpe_pwm->stmpe->partnum == STMPE2401) {
> +			/* STMPE2401 need a complex program */
> +			u16 incdec = 0x0000;
> +
> +			if (stmpe_pwm->lastduty < val)
> +				/* Count up */
> +				incdec = (val - stmpe_pwm->lastduty);
> +			else
> +				/* Count down */
> +				incdec = BIT(7) | (stmpe_pwm->lastduty - val);
> +			/* Step to desired value, smoothly */
> +			program[0] = BIT(14) | BIT(8) | incdec;
> +			/* Loop eternally */
> +			program[1] = BIT(15) | BIT(13);
> +		}
> +
> +		dev_dbg(chip->dev, "PWM%d: val = %02x, lastduty = %02x, "
> +			"program=%04x,%04x,%04x\n", pwm->hwpwm, val,
> +			stmpe_pwm->lastduty, program[0], program[1],
> +			program[2]);
> +		stmpe_pwm->lastduty = val;
> +	}
> +
> +	/*
> +	 * We can write programs of up to 64 16-bit words into this
> +	 * channel.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(program); i++) {
> +		u8 val;
> +
> +		val = (program[i] >> 8) & 0xFF;
> +		ret = stmpe_reg_write(stmpe_pwm->stmpe, reg, val);
> +		if (ret) {
> +			dev_err(chip->dev, "error writing reg\n");

Perhaps you want to say which register you failed to read...

> +			return ret;
> +		}
> +		val = program[i] & 0xFF;
> +		ret = stmpe_reg_write(stmpe_pwm->stmpe, reg, val);
> +		if (ret) {
> +			dev_err(chip->dev, "error writing reg\n");

... and write?

> +			return ret;
> +		}
> +	}
> +	/* If we were enabled, re-enable this PWM */
> +	if (enabled)
> +		stmpe_24xx_pwm_enable(chip, pwm);
> +
> +	/* Sleep for 200ms so we're sure it will take effect */
> +	msleep(200);
> +
> +	dev_dbg(chip->dev, "programmed PWM%d, %d bytes\n",
> +		pwm->hwpwm, i);

pwm->hwpwm is unsigned, so %u. Same for i when you turn that into
unsigned int.

> +	return 0;
> +}
[...]
> +static int stmpe_pwm_probe(struct platform_device *pdev)
> +{
> +	struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
> +	struct stmpe_pwm *pwm;
> +	int ret;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->stmpe = stmpe;
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.base = pdev->id;

Why would you need to hard-code the base? Doesn't dynamic allocation
work for you?

Also, since the device is instantiated as a child of an MFD, won't it
end up with -1 anyway?

> +	if (stmpe->partnum == STMPE2401 || stmpe->partnum == STMPE2403) {
> +		pwm->chip.ops = &stmpe_24xx_pwm_ops;
> +		pwm->chip.npwm = 3;
> +	} else if (stmpe->partnum == STMPE1601) {
> +		dev_err(&pdev->dev, "STMPE1601 PWM not yet supported\n");
> +		return -EINVAL;
> +	} else {
> +		dev_err(&pdev->dev, "Unsupported STMPE PWM\n");
> +		return -EINVAL;
> +	}

Perhaps -ENODEV? Though I don't understand why we need to do this here.
I guess I've become used to enabling support via compatible strings so
much that these constructs look strange to me.

> +
> +	ret = stmpe_enable(stmpe, STMPE_BLOCK_PWM);
> +	if (ret)
> +		return ret;
> +
> +	ret = pwmchip_add(&pwm->chip);
> +	if (ret)
> +		return ret;

Don't you need to disable the PWM block here again?

> +
> +	dev_info(&pdev->dev, "STMPE PWM registered\n");

I don't think this is warranted. Let the user know if there's been an
error, but no need to brag if everything went well. They're supposed to.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2 v2] pwm: add a driver for the STMPE PWM

[Linus Walleij]

This adds a driver for the STMPE 24xx series of multi-purpose
I2C expanders. (I think STMPE means ST Microelectronics
Multi-Purpose Expander.) This PWM was designed in accordance with
Nokia specifications and is kind of weird and usually just
switched between max and zero dutycycle. However it is indeed
a PWM so it needs to live in the PWM subsystem.

This PWM is mostly used for white LED backlight.

Cc: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Use pwm_is_enabled() instead of local state tracking.
- Convert to builtin_platform_driver_probe() and tag the init
  code with _init, we're not going modular with this driver.
- Get rid of some pointless (u8) casts
- Convert an if () else if () else if () ladder into a switch
  statement.
- Make a set of #defines for the PWM state machine instruction
  words.
- Don't brag about the driver being initialized.
ChangeLog v1->v2:
- Split off the MFD hunk from the patch: no need to worry about
  that, it can be merged orthogonally to the MFD tree.
---
 drivers/pwm/Kconfig     |   7 ++
 drivers/pwm/Makefile    |   1 +
 drivers/pwm/pwm-stmpe.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 307 insertions(+)
 create mode 100644 drivers/pwm/pwm-stmpe.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index c182efc62c7b..1a491d094e2e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -362,6 +362,13 @@ config PWM_STI
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sti.
 
+config PWM_STMPE
+	bool "STMPE expander PWM export"
+	depends on MFD_STMPE
+	help
+	  This enables support for the PWMs found in the STMPE I/O
+	  expanders.
+
 config PWM_SUN4I
 	tristate "Allwinner PWM support"
 	depends on ARCH_SUNXI || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index dd35bc121a18..790353b7454e 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
+obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
 obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
diff --git a/drivers/pwm/pwm-stmpe.c b/drivers/pwm/pwm-stmpe.c
new file mode 100644
index 000000000000..e560b583ef28
--- /dev/null
+++ b/drivers/pwm/pwm-stmpe.c
@@ -0,0 +1,299 @@
+/*
+ * Copyright (C) 2016 Linaro Ltd.
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * 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.
+ *
+ */
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/pwm.h>
+#include <linux/of.h>
+#include <linux/mfd/stmpe.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+
+#define STMPE24XX_PWMCS		0x30
+#define PWMCS_EN_PWM0		BIT(0)
+#define PWMCS_EN_PWM1		BIT(1)
+#define PWMCS_EN_PWM2		BIT(2)
+#define STMPE24XX_PWMIC0	0x38
+#define STMPE24XX_PWMIC1	0x39
+#define STMPE24XX_PWMIC2	0x3a
+#define STMPE_PWM_24XX_PINBASE	21
+
+struct stmpe_pwm {
+	struct stmpe *stmpe;
+	struct pwm_chip chip;
+	u8 lastduty;
+};
+
+static inline struct stmpe_pwm *to_stmpe_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct stmpe_pwm, chip);
+}
+
+static int stmpe_24xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
+	int ret;
+	u8 val;
+
+	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
+	if (ret < 0) {
+		dev_err(chip->dev, "error reading PWM%d control\n",
+			pwm->hwpwm);
+		return ret;
+	}
+
+	val = ret;
+	val |= BIT(pwm->hwpwm);
+
+	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, val);
+	if (ret) {
+		dev_err(chip->dev, "error writing PWM%d control\n",
+			pwm->hwpwm);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void stmpe_24xx_pwm_disable(struct pwm_chip *chip,
+				struct pwm_device *pwm)
+{
+	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
+	int ret;
+	u8 val;
+
+	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
+	if (ret < 0) {
+		dev_err(chip->dev, "error reading PWM%d control\n",
+			pwm->hwpwm);
+		return;
+	}
+
+	val = ret;
+	val &= ~BIT(pwm->hwpwm);
+
+	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, val);
+	if (ret) {
+		dev_err(chip->dev, "error writing PWM%d control\n",
+			pwm->hwpwm);
+		return;
+	}
+}
+
+/* STMPE 24xx PWM instructions */
+#define SMAX		0x007F
+#define SMIN		0x00FF
+#define GTS		0x0000
+#define LOAD_2403	BIT(14) /* Only available on 2403 */
+#define RAMPUP		0x0000
+#define RAMPDOWN	BIT(7)
+#define PRESCALE_512	BIT(14)
+#define STEPTIME_1	BIT(8)
+#define BRANCH		BIT(15) | BIT(13)
+
+static int stmpe_24xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+				int duty_ns, int period_ns)
+{
+	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
+	u8 reg;
+	int ret;
+	unsigned int i, pin;
+	u16 program[3] = {
+		SMAX,
+		GTS,
+		GTS,
+	};
+
+	/* Make sure we are disabled */
+	if (pwm_is_enabled(pwm)) {
+		stmpe_24xx_pwm_disable(chip, pwm);
+	} else {
+		/* Connect the PWM to the pin */
+		pin = pwm->hwpwm;
+		/* On STMPE2401 and 2403 pins 21,22,23 are used */
+		if (stmpe_pwm->stmpe->partnum == STMPE2401 ||
+		stmpe_pwm->stmpe->partnum == STMPE2403)
+			pin += STMPE_PWM_24XX_PINBASE;
+		ret = stmpe_set_altfunc(stmpe_pwm->stmpe,
+					BIT(pin),
+					STMPE_BLOCK_PWM);
+		if (ret) {
+			dev_err(chip->dev, "unable to connect PWM%d to pin\n",
+				pwm->hwpwm);
+			return ret;
+		}
+	}
+
+	/* STMPE24XX */
+	switch (pwm->hwpwm) {
+        case 0:
+		reg = STMPE24XX_PWMIC0;
+		break;
+	case 1:
+		reg = STMPE24XX_PWMIC1;
+		break;
+	case 2:
+		reg = STMPE24XX_PWMIC1;
+		break;
+	default:
+		/* Should not happen as npwm is 3 */
+		return -ENODEV;
+	}
+
+	dev_dbg(chip->dev, "PWM%d: config duty %d ns, period %d ns\n",
+		pwm->hwpwm, duty_ns, period_ns);
+	if (duty_ns == 0) {
+		if (stmpe_pwm->stmpe->partnum == STMPE2401)
+			program[0] = SMAX; /* off all the time */
+		if (stmpe_pwm->stmpe->partnum == STMPE2403)
+			program[0] = LOAD_2403 | 0xFF; /* LOAD 0xFF */
+		stmpe_pwm->lastduty = 0x00;
+	} else if (duty_ns == period_ns) {
+		if (stmpe_pwm->stmpe->partnum == STMPE2401)
+			program[0] = SMIN; /* on all the time */
+		if (stmpe_pwm->stmpe->partnum == STMPE2403)
+			program[0] = LOAD_2403 | 0x00; /* LOAD 0x00 */
+		stmpe_pwm->lastduty = 0xFF;
+	} else {
+		unsigned long cyc;
+		u8 val;
+
+		/*
+		 * Counter goes from 0x00 to 0xFF repeatedly at 32768 Hz,
+		 * (means a period of 30517 ns)
+		 * then this is compared to the counter from the ramp,
+		 * if this is >= pwm counter the output is high.
+		 * With LOAD we can define how much of the cycle it is on.
+		 *
+		 * Prescale = 0 -> 2 kHz -> T = 1/f = 488281,25 ns
+		 */
+		/* Scale to 0..FF */
+		cyc = duty_ns * 256;
+		cyc = DIV_ROUND_CLOSEST(cyc, period_ns);
+		val = cyc;
+
+		if (val == stmpe_pwm->lastduty) {
+			/* Run the old program */
+			if (pwm_is_enabled(pwm))
+				stmpe_24xx_pwm_enable(chip, pwm);
+			return 0;
+		} else if (stmpe_pwm->stmpe->partnum == STMPE2403) {
+			/* STMPE2403 can simply set the right PWM value */
+			program[0] = LOAD_2403 | val;
+			program[1] = 0x0000;
+		} else if (stmpe_pwm->stmpe->partnum == STMPE2401) {
+			/* STMPE2401 need a complex program */
+			u16 incdec = 0x0000;
+
+			if (stmpe_pwm->lastduty < val)
+				/* Count up */
+				incdec = RAMPUP | (val - stmpe_pwm->lastduty);
+			else
+				/* Count down */
+				incdec = RAMPDOWN | (stmpe_pwm->lastduty - val);
+			/* Step to desired value, smoothly */
+			program[0] = PRESCALE_512 | STEPTIME_1 | incdec;
+			/* Loop eternally to 0x00 */
+			program[1] = BRANCH;
+		}
+
+		dev_dbg(chip->dev, "PWM%d: val = %02x, lastduty = %02x, "
+			"program=%04x,%04x,%04x\n", pwm->hwpwm, val,
+			stmpe_pwm->lastduty, program[0], program[1],
+			program[2]);
+		stmpe_pwm->lastduty = val;
+	}
+
+	/*
+	 * We can write programs of up to 64 16-bit words into this
+	 * channel.
+	 */
+	for (i = 0; i < ARRAY_SIZE(program); i++) {
+		u8 val;
+
+		val = (program[i] >> 8) & 0xFF;
+		ret = stmpe_reg_write(stmpe_pwm->stmpe, reg, val);
+		if (ret) {
+			dev_err(chip->dev, "error writing reg %02x\n", reg);
+			return ret;
+		}
+		val = program[i] & 0xFF;
+		ret = stmpe_reg_write(stmpe_pwm->stmpe, reg, val);
+		if (ret) {
+			dev_err(chip->dev, "error writing reg %02x\n", reg);
+			return ret;
+		}
+	}
+
+	/* If we were enabled, re-enable this PWM */
+	if (pwm_is_enabled(pwm))
+		stmpe_24xx_pwm_enable(chip, pwm);
+
+	/* Sleep for 200ms so we're sure it will take effect */
+	msleep(200);
+
+	dev_dbg(chip->dev, "programmed PWM%u, %d bytes\n",
+		pwm->hwpwm, i);
+	return 0;
+}
+
+static const struct pwm_ops stmpe_24xx_pwm_ops = {
+	.config = stmpe_24xx_pwm_config,
+	.enable = stmpe_24xx_pwm_enable,
+	.disable = stmpe_24xx_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int __init stmpe_pwm_probe(struct platform_device *pdev)
+{
+	struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
+	struct stmpe_pwm *pwm;
+	int ret;
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->stmpe = stmpe;
+	pwm->chip.dev = &pdev->dev;
+	pwm->chip.base = -1;
+	if (stmpe->partnum == STMPE2401 || stmpe->partnum == STMPE2403) {
+		pwm->chip.ops = &stmpe_24xx_pwm_ops;
+		pwm->chip.npwm = 3;
+	} else if (stmpe->partnum == STMPE1601) {
+		dev_err(&pdev->dev, "STMPE1601 PWM not yet supported\n");
+		return -ENODEV;
+	} else {
+		dev_err(&pdev->dev, "Unsupported STMPE PWM\n");
+		return -ENODEV;
+	}
+
+	ret = stmpe_enable(stmpe, STMPE_BLOCK_PWM);
+	if (ret)
+		return ret;
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, pwm);
+
+	return 0;
+}
+
+static struct platform_driver stmpe_pwm_driver = {
+	.driver = {
+		.name = "stmpe-pwm",
+		.suppress_bind_attrs = true,
+	},
+};
+builtin_platform_driver_probe(stmpe_pwm_driver, stmpe_pwm_probe);
-- 
2.4.11

Re: [PATCH 2/2 v2] pwm: add a driver for the STMPE PWM

[Linus Walleij]

On Tue, Apr 5, 2016 at 11:22 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> ChangeLog v2->v3:

Sorry for the confusion, due to using human intervention.

This is patchset v3, nevermind the Subject line v2...

Yours,
Linus Walleij

Re: [PATCH 2/2 v2] pwm: add a driver for the STMPE PWM

[Linus Walleij]

On Tue, Apr 5, 2016 at 11:22 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> This adds a driver for the STMPE 24xx series of multi-purpose
> I2C expanders. (I think STMPE means ST Microelectronics
> Multi-Purpose Expander.) This PWM was designed in accordance with
> Nokia specifications and is kind of weird and usually just
> switched between max and zero dutycycle. However it is indeed
> a PWM so it needs to live in the PWM subsystem.
>
> This PWM is mostly used for white LED backlight.
>
> Cc: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v2->v3:

Thierry? Soon a month since I posted this and the kernel
merge window is getting closer.

If I need to respin this I'd like to know so I can do it before
the merge window, else can this and 1/2 be applied?

Yours,
Linus Walleij

Re: [PATCH 2/2 v2] pwm: add a driver for the STMPE PWM

[Vladimir Zapolskiy]

Hi Linus,

On 06.04.2016 00:22, Linus Walleij wrote:
> This adds a driver for the STMPE 24xx series of multi-purpose
> I2C expanders. (I think STMPE means ST Microelectronics
> Multi-Purpose Expander.) This PWM was designed in accordance with
> Nokia specifications and is kind of weird and usually just
> switched between max and zero dutycycle. However it is indeed
> a PWM so it needs to live in the PWM subsystem.
> 
> This PWM is mostly used for white LED backlight.
> 
> Cc: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

let me do some shallow review for you.

> ---
> ChangeLog v2->v3:
> - Use pwm_is_enabled() instead of local state tracking.
> - Convert to builtin_platform_driver_probe() and tag the init
>   code with _init, we're not going modular with this driver.
> - Get rid of some pointless (u8) casts
> - Convert an if () else if () else if () ladder into a switch
>   statement.
> - Make a set of #defines for the PWM state machine instruction
>   words.
> - Don't brag about the driver being initialized.
> ChangeLog v1->v2:
> - Split off the MFD hunk from the patch: no need to worry about
>   that, it can be merged orthogonally to the MFD tree.
> ---
>  drivers/pwm/Kconfig     |   7 ++
>  drivers/pwm/Makefile    |   1 +
>  drivers/pwm/pwm-stmpe.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 307 insertions(+)
>  create mode 100644 drivers/pwm/pwm-stmpe.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index c182efc62c7b..1a491d094e2e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -362,6 +362,13 @@ config PWM_STI
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-sti.
>  
> +config PWM_STMPE
> +	bool "STMPE expander PWM export"
> +	depends on MFD_STMPE
> +	help
> +	  This enables support for the PWMs found in the STMPE I/O
> +	  expanders.
> +
>  config PWM_SUN4I
>  	tristate "Allwinner PWM support"
>  	depends on ARCH_SUNXI || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index dd35bc121a18..790353b7454e 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
> +obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
> diff --git a/drivers/pwm/pwm-stmpe.c b/drivers/pwm/pwm-stmpe.c
> new file mode 100644
> index 000000000000..e560b583ef28
> --- /dev/null
> +++ b/drivers/pwm/pwm-stmpe.c
> @@ -0,0 +1,299 @@
> +/*
> + * Copyright (C) 2016 Linaro Ltd.
> + *
> + * Author: Linus Walleij <linus.walleij@linaro.org>
> + *
> + * 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.
> + *
> + */
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/pwm.h>
> +#include <linux/of.h>
> +#include <linux/mfd/stmpe.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>

Here alphabetically sorted list of header files would be better IMHO.

> +
> +#define STMPE24XX_PWMCS		0x30
> +#define PWMCS_EN_PWM0		BIT(0)
> +#define PWMCS_EN_PWM1		BIT(1)
> +#define PWMCS_EN_PWM2		BIT(2)

These 3 bitfields are actually not used in the code.

> +#define STMPE24XX_PWMIC0	0x38
> +#define STMPE24XX_PWMIC1	0x39
> +#define STMPE24XX_PWMIC2	0x3a
> +#define STMPE_PWM_24XX_PINBASE	21
> +
> +struct stmpe_pwm {
> +	struct stmpe *stmpe;
> +	struct pwm_chip chip;
> +	u8 lastduty;
> +};
> +
> +static inline struct stmpe_pwm *to_stmpe_pwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct stmpe_pwm, chip);
> +}
> +
> +static int stmpe_24xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
> +	int ret;
> +	u8 val;
> +
> +	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "error reading PWM%d control\n",
> +			pwm->hwpwm);
> +		return ret;
> +	}
> +
> +	val = ret;
> +	val |= BIT(pwm->hwpwm);
> +
> +	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, val);
> +	if (ret) {
> +		dev_err(chip->dev, "error writing PWM%d control\n",
> +			pwm->hwpwm);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void stmpe_24xx_pwm_disable(struct pwm_chip *chip,
> +				struct pwm_device *pwm)
> +{
> +	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
> +	int ret;
> +	u8 val;
> +
> +	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "error reading PWM%d control\n",
> +			pwm->hwpwm);
> +		return;
> +	}
> +
> +	val = ret;
> +	val &= ~BIT(pwm->hwpwm);
> +
> +	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, val);
> +	if (ret) {
> +		dev_err(chip->dev, "error writing PWM%d control\n",
> +			pwm->hwpwm);
> +		return;
> +	}
> +}

Here *_pwm_disable()/*_pwm_enable() functions are quite verbose, to save
LoC you may consider to add a wrapped *_pwm_enable_disable(..., bool
enable) or so.

> +
> +/* STMPE 24xx PWM instructions */
> +#define SMAX		0x007F
> +#define SMIN		0x00FF
> +#define GTS		0x0000
> +#define LOAD_2403	BIT(14) /* Only available on 2403 */
> +#define RAMPUP		0x0000
> +#define RAMPDOWN	BIT(7)
> +#define PRESCALE_512	BIT(14)
> +#define STEPTIME_1	BIT(8)
> +#define BRANCH		BIT(15) | BIT(13)
> +

May be move macro declarations to the top and add an STMPE 24xx specific prefix?

> +static int stmpe_24xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +				int duty_ns, int period_ns)
> +{
> +	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
> +	u8 reg;
> +	int ret;
> +	unsigned int i, pin;
> +	u16 program[3] = {
> +		SMAX,
> +		GTS,
> +		GTS,
> +	};
> +
> +	/* Make sure we are disabled */
> +	if (pwm_is_enabled(pwm)) {
> +		stmpe_24xx_pwm_disable(chip, pwm);
> +	} else {
> +		/* Connect the PWM to the pin */
> +		pin = pwm->hwpwm;
> +		/* On STMPE2401 and 2403 pins 21,22,23 are used */
> +		if (stmpe_pwm->stmpe->partnum == STMPE2401 ||
> +		stmpe_pwm->stmpe->partnum == STMPE2403)
> +			pin += STMPE_PWM_24XX_PINBASE;
> +		ret = stmpe_set_altfunc(stmpe_pwm->stmpe,
> +					BIT(pin),
> +					STMPE_BLOCK_PWM);
> +		if (ret) {
> +			dev_err(chip->dev, "unable to connect PWM%d to pin\n",
> +				pwm->hwpwm);
> +			return ret;
> +		}
> +	}
> +
> +	/* STMPE24XX */
> +	switch (pwm->hwpwm) {
> +        case 0:

Odd indentation, should be reported by checkpatch, probably there are more
of them.

> +		reg = STMPE24XX_PWMIC0;
> +		break;
> +	case 1:
> +		reg = STMPE24XX_PWMIC1;
> +		break;
> +	case 2:
> +		reg = STMPE24XX_PWMIC1;
> +		break;
> +	default:
> +		/* Should not happen as npwm is 3 */
> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(chip->dev, "PWM%d: config duty %d ns, period %d ns\n",
> +		pwm->hwpwm, duty_ns, period_ns);
> +	if (duty_ns == 0) {
> +		if (stmpe_pwm->stmpe->partnum == STMPE2401)
> +			program[0] = SMAX; /* off all the time */
> +		if (stmpe_pwm->stmpe->partnum == STMPE2403)
> +			program[0] = LOAD_2403 | 0xFF; /* LOAD 0xFF */
> +		stmpe_pwm->lastduty = 0x00;
> +	} else if (duty_ns == period_ns) {
> +		if (stmpe_pwm->stmpe->partnum == STMPE2401)
> +			program[0] = SMIN; /* on all the time */
> +		if (stmpe_pwm->stmpe->partnum == STMPE2403)
> +			program[0] = LOAD_2403 | 0x00; /* LOAD 0x00 */
> +		stmpe_pwm->lastduty = 0xFF;
> +	} else {
> +		unsigned long cyc;
> +		u8 val;
> +
> +		/*
> +		 * Counter goes from 0x00 to 0xFF repeatedly at 32768 Hz,
> +		 * (means a period of 30517 ns)
> +		 * then this is compared to the counter from the ramp,
> +		 * if this is >= pwm counter the output is high.
> +		 * With LOAD we can define how much of the cycle it is on.
> +		 *
> +		 * Prescale = 0 -> 2 kHz -> T = 1/f = 488281,25 ns
> +		 */
> +		/* Scale to 0..FF */
> +		cyc = duty_ns * 256;
> +		cyc = DIV_ROUND_CLOSEST(cyc, period_ns);
> +		val = cyc;

You should be able to do here

   val = DIV_ROUND_CLOSEST(cyc, period_ns);

or even

   val = DIV_ROUND_CLOSEST(duty_ns * 256, period_ns);

if there is no overflow.

> +
> +		if (val == stmpe_pwm->lastduty) {
> +			/* Run the old program */
> +			if (pwm_is_enabled(pwm))
> +				stmpe_24xx_pwm_enable(chip, pwm);
> +			return 0;
> +		} else if (stmpe_pwm->stmpe->partnum == STMPE2403) {

Since the second (and the third) else-if checks for another type of
condition, I would propose to remove chained else-if here, fortunately the
first check ends with return.

> +			/* STMPE2403 can simply set the right PWM value */
> +			program[0] = LOAD_2403 | val;
> +			program[1] = 0x0000;
> +		} else if (stmpe_pwm->stmpe->partnum == STMPE2401) {
> +			/* STMPE2401 need a complex program */
> +			u16 incdec = 0x0000;
> +
> +			if (stmpe_pwm->lastduty < val)
> +				/* Count up */
> +				incdec = RAMPUP | (val - stmpe_pwm->lastduty);
> +			else
> +				/* Count down */
> +				incdec = RAMPDOWN | (stmpe_pwm->lastduty - val);
> +			/* Step to desired value, smoothly */
> +			program[0] = PRESCALE_512 | STEPTIME_1 | incdec;
> +			/* Loop eternally to 0x00 */
> +			program[1] = BRANCH;
> +		}
> +
> +		dev_dbg(chip->dev, "PWM%d: val = %02x, lastduty = %02x, "
> +			"program=%04x,%04x,%04x\n", pwm->hwpwm, val,
> +			stmpe_pwm->lastduty, program[0], program[1],
> +			program[2]);
> +		stmpe_pwm->lastduty = val;
> +	}
> +
> +	/*
> +	 * We can write programs of up to 64 16-bit words into this
> +	 * channel.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(program); i++) {
> +		u8 val;
> +
> +		val = (program[i] >> 8) & 0xFF;
> +		ret = stmpe_reg_write(stmpe_pwm->stmpe, reg, val);
> +		if (ret) {
> +			dev_err(chip->dev, "error writing reg %02x\n", reg);
> +			return ret;
> +		}
> +		val = program[i] & 0xFF;
> +		ret = stmpe_reg_write(stmpe_pwm->stmpe, reg, val);
> +		if (ret) {
> +			dev_err(chip->dev, "error writing reg %02x\n", reg);
> +			return ret;
> +		}
> +	}
> +
> +	/* If we were enabled, re-enable this PWM */
> +	if (pwm_is_enabled(pwm))
> +		stmpe_24xx_pwm_enable(chip, pwm);

I would propose to split stmpe_24xx_pwm_config() into the actual config
function clear from is_enabled()/enable() code and a tiny wrapper which
sets the controller into a defined power state.

> +
> +	/* Sleep for 200ms so we're sure it will take effect */
> +	msleep(200);
> +
> +	dev_dbg(chip->dev, "programmed PWM%u, %d bytes\n",
> +		pwm->hwpwm, i);
> +	return 0;

Empty line before return may decorate the code.

> +}
> +
> +static const struct pwm_ops stmpe_24xx_pwm_ops = {
> +	.config = stmpe_24xx_pwm_config,
> +	.enable = stmpe_24xx_pwm_enable,
> +	.disable = stmpe_24xx_pwm_disable,
> +	.owner = THIS_MODULE,

Is .owner assignment still needed?

> +};
> +
> +static int __init stmpe_pwm_probe(struct platform_device *pdev)
> +{
> +	struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
> +	struct stmpe_pwm *pwm;
> +	int ret;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->stmpe = stmpe;
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.base = -1;
> +	if (stmpe->partnum == STMPE2401 || stmpe->partnum == STMPE2403) {
> +		pwm->chip.ops = &stmpe_24xx_pwm_ops;
> +		pwm->chip.npwm = 3;
> +	} else if (stmpe->partnum == STMPE1601) {
> +		dev_err(&pdev->dev, "STMPE1601 PWM not yet supported\n");
> +		return -ENODEV;
> +	} else {
> +		dev_err(&pdev->dev, "Unsupported STMPE PWM\n");
> +		return -ENODEV;
> +	}

Here I don't see a significant difference between "STMPE1601 PWM not yet
supported" and "Unsupported STMPE PWM", it makes sense to combine them.

> +
> +	ret = stmpe_enable(stmpe, STMPE_BLOCK_PWM);
> +	if (ret)
> +		return ret;
> +
> +	ret = pwmchip_add(&pwm->chip);
> +	if (ret)
> +		return ret;

STMPE_BLOCK_PWM is left enabled on error path.

> +	platform_set_drvdata(pdev, pwm);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver stmpe_pwm_driver = {
> +	.driver = {
> +		.name = "stmpe-pwm",
> +		.suppress_bind_attrs = true,

Hm, I didn't know about this attribute, here its usage seems to
be appropriate.

> +	},
> +};
> +builtin_platform_driver_probe(stmpe_pwm_driver, stmpe_pwm_probe);
> 

--
With best wishes,
Vladimir

Re: [PATCH 2/2 v2] pwm: add a driver for the STMPE PWM

[Thierry Reding]

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

On Tue, Apr 05, 2016 at 11:22:37PM +0200, Linus Walleij wrote:
> This adds a driver for the STMPE 24xx series of multi-purpose
> I2C expanders. (I think STMPE means ST Microelectronics
> Multi-Purpose Expander.) This PWM was designed in accordance with
> Nokia specifications and is kind of weird and usually just
> switched between max and zero dutycycle. However it is indeed
> a PWM so it needs to live in the PWM subsystem.
> 
> This PWM is mostly used for white LED backlight.
> 
> Cc: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v2->v3:
> - Use pwm_is_enabled() instead of local state tracking.
> - Convert to builtin_platform_driver_probe() and tag the init
>   code with _init, we're not going modular with this driver.
> - Get rid of some pointless (u8) casts
> - Convert an if () else if () else if () ladder into a switch
>   statement.
> - Make a set of #defines for the PWM state machine instruction
>   words.
> - Don't brag about the driver being initialized.
> ChangeLog v1->v2:
> - Split off the MFD hunk from the patch: no need to worry about
>   that, it can be merged orthogonally to the MFD tree.
> ---
>  drivers/pwm/Kconfig     |   7 ++
>  drivers/pwm/Makefile    |   1 +
>  drivers/pwm/pwm-stmpe.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 307 insertions(+)
>  create mode 100644 drivers/pwm/pwm-stmpe.c

Sorry, this went completely under my radar. I've applied it now, fixing
up some checkpatch warnings and addressing some of the comments that had
been made by Vladimir.

Linus, can you please check that the driver still works? Or if there's
anything I've changed that you think is completely wrong?

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]