diff mbox series

[v4,4/7] pwm: Add support for Azoteq IQS620A PWM generator

Message ID 1579228475-6681-5-git-send-email-jeff@labundy.com (mailing list archive)
State New, archived
Headers show
Series Add support for Azoteq IQS620A/621/622/624/625 | expand

Commit Message

Jeff LaBundy Jan. 17, 2020, 2:35 a.m. UTC
This patch adds support for the Azoteq IQS620A, capable of generating
a 1-kHz PWM output with duty cycle between ~0.4% and 100% (inclusive).

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v4:
  - Updated iqs620_pwm_apply and iqs620_pwm_get_state to hold the lock around
    only the minimum necessary code
  - Removed the completion protecting against early use of chip->pwms[0] from
    inside iqs620_pwm_notifier in favor of cached register values
  - Updated iqs620_pwm_get_state to use the cached register values instead of
    reading IQS620_PWR_SETTINGS_PWM_OUT and IQS620_PWM_DUTY_CYCLE (both equal
    to zero by default)
  - Added a comment in iqs620_pwm_notifier to note that the parent MFD driver
    prints an error message in the event of a device reset

Changes in v3:
  - Updated the commit message to say "~0.4%" instead of "0.4%"
  - Clarified the effect of duty cycle and state changes in the 'Limitations'
    section and added a restriction regarding 0% duty cycle
  - Added a comment in iqs620_pwm_apply to explain how duty cycle is derived
  - Updated iqs620_pwm_apply to disable the output first and enable it last to
    prevent temporarily driving a stale duty cycle
  - Rounded the calculation for duty cycle up and down in iqs620_pwm_get_state
    and iqs620_pwm_apply, respectively
  - Added a comment in iqs620_pwm_get_state to explain what it reports follow-
    ing requests to set duty cycle to 0%
  - Added a lock to prevent back-to-back access of IQS620_PWR_SETTINGS_PWM_OUT
    and IQS620_PWM_DUTY_CYCLE from being interrupted
  - Updated iqs620_pwm_notifier to reference pwm->state directly as opposed to
    calling pwm_get_state
  - Moved notifier unregistration back to a device-managed action
  - Added a completion to prevent iqs620_pwm_notifier from referencing the
    pwm_chip structure until it has been initialized by pwmchip_add

Changes in v2:
  - Merged 'Copyright' and 'Author' lines into one in introductory comments
  - Added 'Limitations' section to introductory comments
  - Replaced 'error' with 'ret' throughout
  - Added const qualifier to state argument of iqs620_pwm_apply and removed all
    modifications to the variable's contents
  - Updated iqs620_pwm_apply to return -ENOTSUPP or -EINVAL if the requested
    polarity is inverted or the requested period is below 1 ms, respectively
  - Updated iqs620_pwm_apply to disable the PWM output if duty cycle is zero
  - Added iqs620_pwm_get_state
  - Eliminated tabbed alignment of pwm_ops and platform_driver struct members
  - Moved notifier unregistration to already present iqs620_pwm_remove, which
    eliminated the need for a device-managed action and ready flag
  - Added a comment in iqs620_pwm_probe to explain the order of operations
  - Changed Kconfig "depends on" logic to MFD_IQS62X || COMPILE_TEST

 drivers/pwm/Kconfig       |  10 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-iqs620a.c | 258 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 269 insertions(+)
 create mode 100644 drivers/pwm/pwm-iqs620a.c

--
2.7.4

Comments

Uwe Kleine-König Jan. 17, 2020, 7:34 a.m. UTC | #1
On Fri, Jan 17, 2020 at 02:35:57AM +0000, Jeff LaBundy wrote:
> +static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				 struct pwm_state *state)
> +{
> +	struct iqs620_pwm_private *iqs620_pwm;
> +
> +	iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip);
> +
> +	mutex_lock(&iqs620_pwm->lock);
> +
> +	/*
> +	 * Since the device cannot generate a 0% duty cycle, requests to do so
> +	 * cause subsequent calls to iqs620_pwm_get_state to report the output
> +	 * as disabled with duty cycle equal to that which was in use prior to
> +	 * the request. This is not ideal, but is the best compromise based on
> +	 * the capabilities of the device.
> +	 */
> +	state->enabled = iqs620_pwm->out_en;

Hmm, when .get_state is called first (before the first invokation of
.apply) .out_en doesn't represent the hardware's state but is false
unconditionally. This makes it hard to take over a running PWM setup by
the bootloader.

Best regards
Uwe

> +	state->duty_cycle = DIV_ROUND_UP((iqs620_pwm->duty_val + 1) *
> +					 IQS620_PWM_PERIOD_NS, 256);
> +
> +	mutex_unlock(&iqs620_pwm->lock);
> +
> +	state->period = IQS620_PWM_PERIOD_NS;
> +}
Jeff LaBundy Jan. 19, 2020, 11:32 p.m. UTC | #2
Hi Uwe,

Thank you for prompt review.

On Fri, Jan 17, 2020 at 08:34:27AM +0100, Uwe Kleine-König wrote:
> On Fri, Jan 17, 2020 at 02:35:57AM +0000, Jeff LaBundy wrote:
> > +static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +				 struct pwm_state *state)
> > +{
> > +	struct iqs620_pwm_private *iqs620_pwm;
> > +
> > +	iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip);
> > +
> > +	mutex_lock(&iqs620_pwm->lock);
> > +
> > +	/*
> > +	 * Since the device cannot generate a 0% duty cycle, requests to do so
> > +	 * cause subsequent calls to iqs620_pwm_get_state to report the output
> > +	 * as disabled with duty cycle equal to that which was in use prior to
> > +	 * the request. This is not ideal, but is the best compromise based on
> > +	 * the capabilities of the device.
> > +	 */
> > +	state->enabled = iqs620_pwm->out_en;
> 
> Hmm, when .get_state is called first (before the first invokation of
> .apply) .out_en doesn't represent the hardware's state but is false
> unconditionally. This makes it hard to take over a running PWM setup by
> the bootloader.

This was intentional, albeit poorly documented on my part. When the parent
MFD driver probes the device, it issues a soft reset (which returns all of
its registers to their default state). It then loads firmware (essentially
tuning/calibration register settings) and then triggers the device's self-
calibration sequence.

Both IQS620_PWR_SETTINGS_PWM_OUT and IQS620_PWM_DUTY_CYCLE default to zero,
and the firmware does not modify these. Therefore out_en and duty_val match
the hardware even before iqs620_pwm_apply is called, as they're initialized
to zero as well.

I would be happy to add a comment in iqs620_pwm_get_state describing this
behavior; I should have described it there rather than the review history
(sorry about that).

However, you bring up a really interesting point about preserving what may
have been done by the bootloader. The device holds itself in POR until its
supply reaches a sufficient level, so there isn't necessarily a functional
reason to manually issue a soft reset from the parent MFD driver.

I could get rid of the manual soft reset, and then simply sync both out_en
and duty_val in iqs620_pwm_probe which would allow iqs620_pwm_get_state to
pick up any changes made by the bootloader prior to the kernel coming up.

The only problem is that leds-pwm disables the pwm at start-up, so the end
result is the same anyway. Regardless of the behavior of any one consumer,
however, I'm slightly inclined to go with the second option as it seems to
be less restrictive and more maintainable. Let me know if you disagree.

> 
> Best regards
> Uwe
> 
> > +	state->duty_cycle = DIV_ROUND_UP((iqs620_pwm->duty_val + 1) *
> > +					 IQS620_PWM_PERIOD_NS, 256);
> > +
> > +	mutex_unlock(&iqs620_pwm->lock);
> > +
> > +	state->period = IQS620_PWM_PERIOD_NS;
> > +}
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Kind regards,
Jeff LaBundy
Uwe Kleine-König Jan. 20, 2020, 7:27 a.m. UTC | #3
Hello Jeff,

On Sun, Jan 19, 2020 at 11:32:39PM +0000, Jeff LaBundy wrote:
> On Fri, Jan 17, 2020 at 08:34:27AM +0100, Uwe Kleine-König wrote:
> > On Fri, Jan 17, 2020 at 02:35:57AM +0000, Jeff LaBundy wrote:
> > > +static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +				 struct pwm_state *state)
> > > +{
> > > +	struct iqs620_pwm_private *iqs620_pwm;
> > > +
> > > +	iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip);
> > > +
> > > +	mutex_lock(&iqs620_pwm->lock);
> > > +
> > > +	/*
> > > +	 * Since the device cannot generate a 0% duty cycle, requests to do so
> > > +	 * cause subsequent calls to iqs620_pwm_get_state to report the output
> > > +	 * as disabled with duty cycle equal to that which was in use prior to
> > > +	 * the request. This is not ideal, but is the best compromise based on
> > > +	 * the capabilities of the device.
> > > +	 */
> > > +	state->enabled = iqs620_pwm->out_en;
> > 
> > Hmm, when .get_state is called first (before the first invokation of
> > .apply) .out_en doesn't represent the hardware's state but is false
> > unconditionally. This makes it hard to take over a running PWM setup by
> > the bootloader.
> 
> This was intentional, albeit poorly documented on my part. When the parent
> MFD driver probes the device, it issues a soft reset (which returns all of
> its registers to their default state). It then loads firmware (essentially
> tuning/calibration register settings) and then triggers the device's self-
> calibration sequence.
> 
> Both IQS620_PWR_SETTINGS_PWM_OUT and IQS620_PWM_DUTY_CYCLE default to zero,
> and the firmware does not modify these. Therefore out_en and duty_val match
> the hardware even before iqs620_pwm_apply is called, as they're initialized
> to zero as well.
> 
> I would be happy to add a comment in iqs620_pwm_get_state describing this
> behavior; I should have described it there rather than the review history
> (sorry about that).

I didn't saw that mentioned in the review history, just skimmed through
the driver.

> However, you bring up a really interesting point about preserving what may
> have been done by the bootloader. The device holds itself in POR until its
> supply reaches a sufficient level, so there isn't necessarily a functional
> reason to manually issue a soft reset from the parent MFD driver.
> 
> I could get rid of the manual soft reset, and then simply sync both out_en
> and duty_val in iqs620_pwm_probe which would allow iqs620_pwm_get_state to
> pick up any changes made by the bootloader prior to the kernel coming up.

That sounds good. This way the PWM driver is independent of the MFD
driver and does the right thing no matter if parent resets the chip or
not.
 
> The only problem is that leds-pwm disables the pwm at start-up, so the end
> result is the same anyway. Regardless of the behavior of any one consumer,
> however, I'm slightly inclined to go with the second option as it seems to
> be less restrictive and more maintainable. Let me know if you disagree.

With

	default-state = "keep";

in your dt the LED shouldn't get disabled.

Best regards
Uwe
Jeff LaBundy Jan. 22, 2020, 3:56 a.m. UTC | #4
Hi Uwe,

> > However, you bring up a really interesting point about preserving what may
> > have been done by the bootloader. The device holds itself in POR until its
> > supply reaches a sufficient level, so there isn't necessarily a functional
> > reason to manually issue a soft reset from the parent MFD driver.
> > 
> > I could get rid of the manual soft reset, and then simply sync both out_en
> > and duty_val in iqs620_pwm_probe which would allow iqs620_pwm_get_state to
> > pick up any changes made by the bootloader prior to the kernel coming up.
> 
> That sounds good. This way the PWM driver is independent of the MFD
> driver and does the right thing no matter if parent resets the chip or
> not.

Agreed on all counts.

>  
> > The only problem is that leds-pwm disables the pwm at start-up, so the end
> > result is the same anyway. Regardless of the behavior of any one consumer,
> > however, I'm slightly inclined to go with the second option as it seems to
> > be less restrictive and more maintainable. Let me know if you disagree.
> 
> With
> 
> 	default-state = "keep";
> 
> in your dt the LED shouldn't get disabled.

I see default-state defined as a common LED property, but leds-pwm doesn't
seem to use it unfortunately. Looking through its code, brightness is just
initialized to zero unconditionally.

This doesn't change what is the right thing to do, nor do I imagine it to
be a problem for typical use cases, just noting for completeness (however
if I am mistaken please let me know).

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Kind regards,
Jeff LaBundy
Uwe Kleine-König Jan. 22, 2020, 7:02 a.m. UTC | #5
Hello Jeff,

On Wed, Jan 22, 2020 at 03:56:14AM +0000, Jeff LaBundy wrote:
> > > The only problem is that leds-pwm disables the pwm at start-up, so the end
> > > result is the same anyway. Regardless of the behavior of any one consumer,
> > > however, I'm slightly inclined to go with the second option as it seems to
> > > be less restrictive and more maintainable. Let me know if you disagree.
> > 
> > With
> > 
> > 	default-state = "keep";
> > 
> > in your dt the LED shouldn't get disabled.
> 
> I see default-state defined as a common LED property, but leds-pwm doesn't
> seem to use it unfortunately. Looking through its code, brightness is just
> initialized to zero unconditionally.

Sounds like a bug.

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index bd21655..60bcf6c 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -222,6 +222,16 @@  config PWM_IMX_TPM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-imx-tpm.

+config PWM_IQS620A
+	tristate "Azoteq IQS620A PWM support"
+	depends on MFD_IQS62X || COMPILE_TEST
+	help
+	  Generic PWM framework driver for the Azoteq IQS620A multi-function
+	  sensor.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called pwm-iqs620a.
+
 config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
 	depends on MACH_INGENIC
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9a47507..a59c710 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -20,6 +20,7 @@  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
 obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
 obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
 obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
+obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
 obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
new file mode 100644
index 0000000..75ca482
--- /dev/null
+++ b/drivers/pwm/pwm-iqs620a.c
@@ -0,0 +1,258 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Azoteq IQS620A PWM Generator
+ *
+ * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com>
+ *
+ * Limitations:
+ * - The period is fixed to 1 ms and is generated continuously despite changes
+ *   to the duty cycle or enable/disable state.
+ * - Changes to the duty cycle or enable/disable state take effect immediately
+ *   and may result in a glitch during the period in which the change is made.
+ * - The device cannot generate a 0% duty cycle. For duty cycles below 1 / 256
+ *   ms, the output is disabled and relies upon an external pull-down resistor
+ *   to hold the GPIO3/LTX pin low.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/mfd/iqs62x.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define IQS620_PWR_SETTINGS			0xD2
+#define IQS620_PWR_SETTINGS_PWM_OUT		BIT(7)
+
+#define IQS620_PWM_DUTY_CYCLE			0xD8
+
+#define IQS620_PWM_PERIOD_NS			1000000
+
+struct iqs620_pwm_private {
+	struct iqs62x_core *iqs62x;
+	struct pwm_chip chip;
+	struct notifier_block notifier;
+	struct mutex lock;
+	bool out_en;
+	u8 duty_val;
+};
+
+static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	struct iqs620_pwm_private *iqs620_pwm;
+	struct iqs62x_core *iqs62x;
+	int duty_scale, ret;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -ENOTSUPP;
+
+	if (state->period < IQS620_PWM_PERIOD_NS)
+		return -EINVAL;
+
+	iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip);
+	iqs62x = iqs620_pwm->iqs62x;
+
+	/*
+	 * The duty cycle generated by the device is calculated as follows:
+	 *
+	 * duty_cycle = (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms
+	 *
+	 * ...where IQS620_PWM_DUTY_CYCLE is a register value between 0 and 255
+	 * (inclusive). Therefore the lowest duty cycle the device can generate
+	 * while the output is enabled is 1 / 256 ms.
+	 *
+	 * For lower duty cycles (e.g. 0), the PWM output is simply disabled to
+	 * allow an on-board pull-down resistor to hold the GPIO3/LTX pin low.
+	 */
+	duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS;
+
+	mutex_lock(&iqs620_pwm->lock);
+
+	if (!state->enabled || !duty_scale) {
+		ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS,
+					 IQS620_PWR_SETTINGS_PWM_OUT, 0);
+		if (ret)
+			goto err_mutex;
+	}
+
+	if (duty_scale) {
+		u8 duty_val = min(duty_scale - 1, 0xFF);
+
+		ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE,
+				   duty_val);
+		if (ret)
+			goto err_mutex;
+
+		iqs620_pwm->duty_val = duty_val;
+	}
+
+	if (state->enabled && duty_scale) {
+		ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS,
+					 IQS620_PWR_SETTINGS_PWM_OUT, 0xFF);
+		if (ret)
+			goto err_mutex;
+	}
+
+	iqs620_pwm->out_en = state->enabled;
+
+err_mutex:
+	mutex_unlock(&iqs620_pwm->lock);
+
+	return ret;
+}
+
+static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	struct iqs620_pwm_private *iqs620_pwm;
+
+	iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip);
+
+	mutex_lock(&iqs620_pwm->lock);
+
+	/*
+	 * Since the device cannot generate a 0% duty cycle, requests to do so
+	 * cause subsequent calls to iqs620_pwm_get_state to report the output
+	 * as disabled with duty cycle equal to that which was in use prior to
+	 * the request. This is not ideal, but is the best compromise based on
+	 * the capabilities of the device.
+	 */
+	state->enabled = iqs620_pwm->out_en;
+	state->duty_cycle = DIV_ROUND_UP((iqs620_pwm->duty_val + 1) *
+					 IQS620_PWM_PERIOD_NS, 256);
+
+	mutex_unlock(&iqs620_pwm->lock);
+
+	state->period = IQS620_PWM_PERIOD_NS;
+}
+
+static int iqs620_pwm_notifier(struct notifier_block *notifier,
+			       unsigned long event_flags, void *context)
+{
+	struct iqs620_pwm_private *iqs620_pwm;
+	struct iqs62x_core *iqs62x;
+	int ret;
+
+	if (!(event_flags & BIT(IQS62X_EVENT_SYS_RESET)))
+		return NOTIFY_DONE;
+
+	iqs620_pwm = container_of(notifier, struct iqs620_pwm_private,
+				  notifier);
+	iqs62x = iqs620_pwm->iqs62x;
+
+	mutex_lock(&iqs620_pwm->lock);
+
+	/*
+	 * The parent MFD driver already prints an error message in the event
+	 * of a device reset, so nothing else is printed here unless there is
+	 * an additional failure.
+	 */
+	ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE,
+			   iqs620_pwm->duty_val);
+	if (ret)
+		goto err_mutex;
+
+	ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS,
+				 IQS620_PWR_SETTINGS_PWM_OUT,
+				 iqs620_pwm->out_en ? 0xFF : 0);
+
+err_mutex:
+	mutex_unlock(&iqs620_pwm->lock);
+
+	if (ret) {
+		dev_err(iqs620_pwm->chip.dev,
+			"Failed to re-initialize device: %d\n", ret);
+		return NOTIFY_BAD;
+	}
+
+	return NOTIFY_OK;
+}
+
+static const struct pwm_ops iqs620_pwm_ops = {
+	.apply = iqs620_pwm_apply,
+	.get_state = iqs620_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static void iqs620_pwm_notifier_unregister(void *context)
+{
+	struct iqs620_pwm_private *iqs620_pwm = context;
+	int ret;
+
+	ret = blocking_notifier_chain_unregister(&iqs620_pwm->iqs62x->nh,
+						 &iqs620_pwm->notifier);
+	if (ret)
+		dev_err(iqs620_pwm->chip.dev,
+			"Failed to unregister notifier: %d\n", ret);
+}
+
+static int iqs620_pwm_probe(struct platform_device *pdev)
+{
+	struct iqs620_pwm_private *iqs620_pwm;
+	int ret;
+
+	iqs620_pwm = devm_kzalloc(&pdev->dev, sizeof(*iqs620_pwm), GFP_KERNEL);
+	if (!iqs620_pwm)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, iqs620_pwm);
+	iqs620_pwm->iqs62x = dev_get_drvdata(pdev->dev.parent);
+
+	iqs620_pwm->chip.dev = &pdev->dev;
+	iqs620_pwm->chip.ops = &iqs620_pwm_ops;
+	iqs620_pwm->chip.base = -1;
+	iqs620_pwm->chip.npwm = 1;
+
+	mutex_init(&iqs620_pwm->lock);
+
+	iqs620_pwm->notifier.notifier_call = iqs620_pwm_notifier;
+	ret = blocking_notifier_chain_register(&iqs620_pwm->iqs62x->nh,
+					       &iqs620_pwm->notifier);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       iqs620_pwm_notifier_unregister,
+				       iqs620_pwm);
+	if (ret)
+		return ret;
+
+	ret = pwmchip_add(&iqs620_pwm->chip);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to add device: %d\n", ret);
+
+	return ret;
+}
+
+static int iqs620_pwm_remove(struct platform_device *pdev)
+{
+	struct iqs620_pwm_private *iqs620_pwm = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&iqs620_pwm->chip);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to remove device: %d\n", ret);
+
+	return ret;
+}
+
+static struct platform_driver iqs620_pwm_platform_driver = {
+	.driver = {
+		.name = IQS620_DRV_NAME_PWM,
+	},
+	.probe = iqs620_pwm_probe,
+	.remove = iqs620_pwm_remove,
+};
+module_platform_driver(iqs620_pwm_platform_driver);
+
+MODULE_AUTHOR("Jeff LaBundy <jeff@labundy.com>");
+MODULE_DESCRIPTION("Azoteq IQS620A PWM Generator");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" IQS620_DRV_NAME_PWM);