[PATCH] pwm: add support for Intel Low Power Subsystem PWM

[PATCH] pwm: add support for Intel Low Power Subsystem PWM

[Chew Chiau Ee]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Add support for Intel Low Power I/O subsystem PWM controllers found on
Intel BayTrail SoC.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Chew, Kean Ho <kean.ho.chew@intel.com>
Signed-off-by: Chang, Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
---
 drivers/pwm/Kconfig    |   10 +++
 drivers/pwm/Makefile   |    1 +
 drivers/pwm/pwm-lpss.c |  207 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 218 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/pwm-lpss.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 6a2a1e0a..4f75589 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -109,6 +109,16 @@ config PWM_LPC32XX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-lpc32xx.
 
+config PWM_LPSS
+	tristate "Intel LPSS PWM support"
+	depends on ACPI
+	help
+	  Generic PWM framework driver for Intel Low Power Subsystem PWM
+	  controller.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-lpss.
+
 config PWM_MXS
 	tristate "Freescale MXS PWM support"
 	depends on ARCH_MXS && OF
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 1b99cfb..d5b316f 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
+obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
new file mode 100644
index 0000000..a7418ca
--- /dev/null
+++ b/drivers/pwm/pwm-lpss.c
@@ -0,0 +1,207 @@
+/*
+ * Intel Low Power Subsystem PWM controller driver
+ *
+ * Copyright (C) 2014, Intel Corporation
+ * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
+ * Author: Chew Kean Ho <kean.ho.chew@intel.com>
+ * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
+ * Author: Chew Chiau Ee <chiau.ee.chew@intel.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.
+ */
+
+#include <linux/acpi.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pwm.h>
+#include <linux/platform_device.h>
+
+#define PWM				0x00000000
+#define PWM_ENABLE			BIT(31)
+#define PWM_SW_UPDATE			BIT(30)
+#define PWM_BASE_UNIT_SHIFT		8
+#define PWM_BASE_UNIT_MASK		0x00ffff00
+#define PWM_ON_TIME_DIV_MASK		0x000000ff
+#define PWM_DIVISION_CORRECTION		0x2
+#define PWM_LIMIT			(0x8000 + PWM_DIVISION_CORRECTION)
+
+
+struct pwm_lpss_chip {
+	struct pwm_chip chip;
+	void __iomem *regs;
+	struct clk *clk;
+};
+
+static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct pwm_lpss_chip, chip);
+}
+
+static void pwm_lpss_set_state(struct pwm_lpss_chip *lpwm, bool enable)
+{
+	u32 ctrl;
+
+	ctrl = readl(lpwm->regs + PWM);
+	if (enable)
+		ctrl |= PWM_ENABLE;
+	else
+		ctrl &= ~PWM_ENABLE;
+	writel(ctrl, lpwm->regs + PWM);
+}
+
+static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
+	int duty_ns, int period_ns)
+{
+	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
+	u8 on_time_div;
+	unsigned long c = clk_get_rate(lpwm->clk);
+	unsigned long long base_unit, hz = 1000000000UL;
+	u32 ctrl;
+
+	do_div(hz, period_ns);
+
+	/* The equation is: base_unit = ((hz / c) * 65536) + correction */
+	base_unit = hz * 65536;
+	do_div(base_unit, c);
+	base_unit += PWM_DIVISION_CORRECTION;
+	if (base_unit > PWM_LIMIT)
+		return -EINVAL;
+
+	if (duty_ns <= 0)
+		duty_ns = 1;
+	on_time_div = 255 - (255 * duty_ns / period_ns);
+
+	ctrl = readl(lpwm->regs + PWM);
+	ctrl &= ~(PWM_BASE_UNIT_MASK | PWM_ON_TIME_DIV_MASK);
+	ctrl |= (u16) base_unit << PWM_BASE_UNIT_SHIFT;
+	ctrl |= on_time_div;
+	/* request PWM to update on next cycle */
+	ctrl |= PWM_SW_UPDATE;
+	writel(ctrl, lpwm->regs + PWM);
+
+	return 0;
+}
+
+static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
+
+	clk_prepare_enable(lpwm->clk);
+	pwm_lpss_set_state(lpwm, true);
+	return 0;
+}
+
+static void pwm_lpss_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
+
+	pwm_lpss_set_state(lpwm, false);
+	clk_disable_unprepare(lpwm->clk);
+}
+
+static const struct pwm_ops pwm_lpss_ops = {
+	.config = pwm_lpss_config,
+	.enable = pwm_lpss_enable,
+	.disable = pwm_lpss_disable,
+	.owner = THIS_MODULE,
+};
+
+#ifdef CONFIG_ACPI
+struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev)
+{
+	struct pwm_lpss_chip *lpwm;
+	struct resource *r;
+
+	lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL);
+	if (!lpwm) {
+		dev_err(&pdev->dev, "failed to allocate memory for platform data\n");
+		return NULL;
+	}
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r) {
+		dev_err(&pdev->dev, "failed to get mmio base\n");
+		return NULL;
+	}
+
+	lpwm->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(lpwm->clk)) {
+		dev_err(&pdev->dev, "failed to get pwm clk\n");
+		return NULL;
+	}
+
+	lpwm->chip.base = -1;
+
+	lpwm->regs = devm_request_and_ioremap(&pdev->dev, r);
+	if (!lpwm->regs)
+		return NULL;
+
+	return lpwm;
+}
+
+static const struct acpi_device_id pwm_lpss_acpi_match[] = {
+	{ "80860F08", 0 },
+	{ "80860F09", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match);
+#else
+struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev)
+{
+	return NULL;
+}
+#endif
+
+static int pwm_lpss_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pwm_lpss_chip *lpwm;
+	int ret;
+
+	lpwm = dev_get_platdata(dev);
+	if (!lpwm) {
+		lpwm = pwm_lpss_acpi_get_pdata(pdev);
+		if (!lpwm)
+			return -ENODEV;
+	}
+
+	lpwm->chip.dev = &pdev->dev;
+	lpwm->chip.ops = &pwm_lpss_ops;
+	lpwm->chip.npwm = 1;
+
+	ret = pwmchip_add(&lpwm->chip);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, lpwm);
+	return 0;
+}
+
+static int pwm_lpss_remove(struct platform_device *pdev)
+{
+	struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev);
+
+	pwm_lpss_set_state(lpwm, false);
+	return pwmchip_remove(&lpwm->chip);
+}
+
+static struct platform_driver pwm_lpss_driver = {
+	.driver = {
+		.name = "pwm-lpss",
+		.acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match),
+	},
+	.probe = pwm_lpss_probe,
+	.remove = pwm_lpss_remove,
+};
+module_platform_driver(pwm_lpss_driver);
+
+MODULE_DESCRIPTION("PWM driver for Intel LPSS");
+MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pwm-lpss");
-- 
1.7.4.4

Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[One Thousand Gnomes]

> +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +	int duty_ns, int period_ns)
> +{
> +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> +	u8 on_time_div;
> +	unsigned long c = clk_get_rate(lpwm->clk);
> +	unsigned long long base_unit, hz = 1000000000UL;
> +	u32 ctrl;
> +
> +	do_div(hz, period_ns);
> +
> +	/* The equation is: base_unit = ((hz / c) * 65536) + correction */
> +	base_unit = hz * 65536;
> +	do_div(base_unit, c);
> +	base_unit += PWM_DIVISION_CORRECTION;
> +	if (base_unit > PWM_LIMIT)
> +		return -EINVAL;
> +
> +	if (duty_ns <= 0)
> +		duty_ns = 1;
> +	on_time_div = 255 - (255 * duty_ns / period_ns);
> +
> +	ctrl = readl(lpwm->regs + PWM);
> +	ctrl &= ~(PWM_BASE_UNIT_MASK | PWM_ON_TIME_DIV_MASK);
> +	ctrl |= (u16) base_unit << PWM_BASE_UNIT_SHIFT;
> +	ctrl |= on_time_div;
> +	/* request PWM to update on next cycle */
> +	ctrl |= PWM_SW_UPDATE;
> +	writel(ctrl, lpwm->regs + PWM);
> +

Who handles the locking on all these functions. The pwm layer doesn't but
simnply exports stuff like pwm_config() directly to other bits of the
kernel so you are not guaranteed to be called via sysfs.

(This btw looks to be a problem with a pile of the other pwm drivers, and
with the pwm core code which doesn't properly lock its own handling of
pwm->duty_cycle and pwm->period in pwm_config(), nor pwm->polarity in
pwm_set_polarity).

I think the core config methods need some kind of locking ?

Alan

Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[Mika Westerberg]

On Mon, Jan 20, 2014 at 01:28:14PM +0000, One Thousand Gnomes wrote:
> > +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +	int duty_ns, int period_ns)
> > +{
> > +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> > +	u8 on_time_div;
> > +	unsigned long c = clk_get_rate(lpwm->clk);
> > +	unsigned long long base_unit, hz = 1000000000UL;
> > +	u32 ctrl;
> > +
> > +	do_div(hz, period_ns);
> > +
> > +	/* The equation is: base_unit = ((hz / c) * 65536) + correction */
> > +	base_unit = hz * 65536;
> > +	do_div(base_unit, c);
> > +	base_unit += PWM_DIVISION_CORRECTION;
> > +	if (base_unit > PWM_LIMIT)
> > +		return -EINVAL;
> > +
> > +	if (duty_ns <= 0)
> > +		duty_ns = 1;
> > +	on_time_div = 255 - (255 * duty_ns / period_ns);
> > +
> > +	ctrl = readl(lpwm->regs + PWM);
> > +	ctrl &= ~(PWM_BASE_UNIT_MASK | PWM_ON_TIME_DIV_MASK);
> > +	ctrl |= (u16) base_unit << PWM_BASE_UNIT_SHIFT;
> > +	ctrl |= on_time_div;
> > +	/* request PWM to update on next cycle */
> > +	ctrl |= PWM_SW_UPDATE;
> > +	writel(ctrl, lpwm->regs + PWM);
> > +
> 
> Who handles the locking on all these functions. The pwm layer doesn't but
> simnply exports stuff like pwm_config() directly to other bits of the
> kernel so you are not guaranteed to be called via sysfs.
> 
> (This btw looks to be a problem with a pile of the other pwm drivers, and
> with the pwm core code which doesn't properly lock its own handling of
> pwm->duty_cycle and pwm->period in pwm_config(), nor pwm->polarity in
> pwm_set_polarity).

Good point. I checked few PWM drivers as well and none of them (including
this one) does any locking around ->config().

> I think the core config methods need some kind of locking ?

Thierry, any comments on this?

Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[Thierry Reding]

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

On Tue, Jan 21, 2014 at 10:52:36AM +0200, Mika Westerberg wrote:
> On Mon, Jan 20, 2014 at 01:28:14PM +0000, One Thousand Gnomes wrote:
> > > +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +	int duty_ns, int period_ns)
> > > +{
> > > +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> > > +	u8 on_time_div;
> > > +	unsigned long c = clk_get_rate(lpwm->clk);
> > > +	unsigned long long base_unit, hz = 1000000000UL;
> > > +	u32 ctrl;
> > > +
> > > +	do_div(hz, period_ns);
> > > +
> > > +	/* The equation is: base_unit = ((hz / c) * 65536) + correction */
> > > +	base_unit = hz * 65536;
> > > +	do_div(base_unit, c);
> > > +	base_unit += PWM_DIVISION_CORRECTION;
> > > +	if (base_unit > PWM_LIMIT)
> > > +		return -EINVAL;
> > > +
> > > +	if (duty_ns <= 0)
> > > +		duty_ns = 1;
> > > +	on_time_div = 255 - (255 * duty_ns / period_ns);
> > > +
> > > +	ctrl = readl(lpwm->regs + PWM);
> > > +	ctrl &= ~(PWM_BASE_UNIT_MASK | PWM_ON_TIME_DIV_MASK);
> > > +	ctrl |= (u16) base_unit << PWM_BASE_UNIT_SHIFT;
> > > +	ctrl |= on_time_div;
> > > +	/* request PWM to update on next cycle */
> > > +	ctrl |= PWM_SW_UPDATE;
> > > +	writel(ctrl, lpwm->regs + PWM);
> > > +
> > 
> > Who handles the locking on all these functions. The pwm layer doesn't but
> > simnply exports stuff like pwm_config() directly to other bits of the
> > kernel so you are not guaranteed to be called via sysfs.
> > 
> > (This btw looks to be a problem with a pile of the other pwm drivers, and
> > with the pwm core code which doesn't properly lock its own handling of
> > pwm->duty_cycle and pwm->period in pwm_config(), nor pwm->polarity in
> > pwm_set_polarity).
> 
> Good point. I checked few PWM drivers as well and none of them (including
> this one) does any locking around ->config().
> 
> > I think the core config methods need some kind of locking ?
> 
> Thierry, any comments on this?

The idea behind this is that only a single user can have access to a
given PWM device at a time. The PWM device's PWMF_REQUESTED flag is set
(and cleared) under the pwm_lock and any subsequent users will not be
able to use that specific device (pwm_request() return -EBUSY).

There is obviously an assumption here that each user knows what they are
doing and aren't calling any of the public pwm_*() functions
concurrently. I haven't come across a situation where this is actually a
problem because typically these functions are called either via sysfs or
some other higher-level where synchronization is already properly
handled.

So the only thing that drivers should be taking care of is synchronizing
access to registers common to multiple PWM devices.

Does that clarify things?

Thierry

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

Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[Mika Westerberg]

On Tue, Jan 21, 2014 at 08:43:39PM +0100, Thierry Reding wrote:
> The idea behind this is that only a single user can have access to a
> given PWM device at a time. The PWM device's PWMF_REQUESTED flag is set
> (and cleared) under the pwm_lock and any subsequent users will not be
> able to use that specific device (pwm_request() return -EBUSY).
> 
> There is obviously an assumption here that each user knows what they are
> doing and aren't calling any of the public pwm_*() functions
> concurrently. I haven't come across a situation where this is actually a
> problem because typically these functions are called either via sysfs or
> some other higher-level where synchronization is already properly
> handled.
> 
> So the only thing that drivers should be taking care of is synchronizing
> access to registers common to multiple PWM devices.

OK, and since LPSS PWM don't share registers we shouldn't need to do
anything here.

> Does that clarify things?

It does for me, thanks for the explanation.

RE: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[Chew, Chiau Ee]

> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: Wednesday, January 22, 2014 5:31 PM
> To: Thierry Reding
> Cc: One Thousand Gnomes; Chew, Chiau Ee; linux-pwm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Chew, Kean Ho; Chang, Rebecca Swee Fun
> Subject: Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM
> 
> On Tue, Jan 21, 2014 at 08:43:39PM +0100, Thierry Reding wrote:
> > The idea behind this is that only a single user can have access to a
> > given PWM device at a time. The PWM device's PWMF_REQUESTED flag is
> > set (and cleared) under the pwm_lock and any subsequent users will not
> > be able to use that specific device (pwm_request() return -EBUSY).
> >
> > There is obviously an assumption here that each user knows what they
> > are doing and aren't calling any of the public pwm_*() functions
> > concurrently. I haven't come across a situation where this is actually
> > a problem because typically these functions are called either via
> > sysfs or some other higher-level where synchronization is already
> > properly handled.
> >
> > So the only thing that drivers should be taking care of is
> > synchronizing access to registers common to multiple PWM devices.
> 
> OK, and since LPSS PWM don't share registers we shouldn't need to do anything
> here.
> 
> > Does that clarify things?
> 
> It does for me, thanks for the explanation.

Hi Thierry,

Would like to find out whether this pwm patch ready to get accepted into mainline kernel? Didn't mean to be pushy :).
Btw, in order to have the Intel Baytrail PWM controller working, it has dependency on the acpi_lpss.c which I have sent the patch (https://lkml.org/lkml/2014/2/18/127) for upstream as well and cc you in the loop. 

Thanks,
Chiau Ee

Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[One Thousand Gnomes]

> Does that clarify things?

Yes

Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[Thierry Reding]

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

On Tue, Jan 21, 2014 at 02:00:08AM +0800, Chew Chiau Ee wrote:
[...]
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
[...]
> +/*
> + * Intel Low Power Subsystem PWM controller driver
> + *
> + * Copyright (C) 2014, Intel Corporation
> + * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
> + * Author: Chew Kean Ho <kean.ho.chew@intel.com>
> + * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> + * Author: Chew Chiau Ee <chiau.ee.chew@intel.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.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pwm.h>
> +#include <linux/platform_device.h>
> +
> +#define PWM				0x00000000
> +#define PWM_ENABLE			BIT(31)
> +#define PWM_SW_UPDATE			BIT(30)
> +#define PWM_BASE_UNIT_SHIFT		8
> +#define PWM_BASE_UNIT_MASK		0x00ffff00
> +#define PWM_ON_TIME_DIV_MASK		0x000000ff

Does the documentation really call this "on time"? I've always only seen
this called duty-cycle.

> +#define PWM_DIVISION_CORRECTION		0x2
> +#define PWM_LIMIT			(0x8000 + PWM_DIVISION_CORRECTION)
> +
> +
> +struct pwm_lpss_chip {
> +	struct pwm_chip chip;
> +	void __iomem *regs;
> +	struct clk *clk;
> +};
> +
> +static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct pwm_lpss_chip, chip);
> +}
> +
> +static void pwm_lpss_set_state(struct pwm_lpss_chip *lpwm, bool enable)
> +{
> +	u32 ctrl;
> +
> +	ctrl = readl(lpwm->regs + PWM);
> +	if (enable)
> +		ctrl |= PWM_ENABLE;
> +	else
> +		ctrl &= ~PWM_ENABLE;
> +	writel(ctrl, lpwm->regs + PWM);

Nit: could use more blank lines around readl() and writel(), but I'm
told that not everybody likes it that way, so if you absolutely must
keep it this way, that's fine, too. =)

Also, is it really necessary to turn this into a function? It's only
called in three places, where each call site would only require three
lines. This function takes up 12 lines in total, so there's no real
gain.

> +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +	int duty_ns, int period_ns)

Align arguments on subsequent lines with those of the first line,
please.

> +{
> +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> +	u8 on_time_div;
> +	unsigned long c = clk_get_rate(lpwm->clk);
> +	unsigned long long base_unit, hz = 1000000000UL;

"hz" -> "freq"? "1000000000UL" -> "NSECS_PER_SEC"?

> +static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> +
> +	clk_prepare_enable(lpwm->clk);

You need to check the return value of clk_prepare_enable().

> +#ifdef CONFIG_ACPI
> +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev)
> +{
> +	struct pwm_lpss_chip *lpwm;
> +	struct resource *r;
> +
> +	lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL);
> +	if (!lpwm) {
> +		dev_err(&pdev->dev, "failed to allocate memory for platform data\n");

No need to print this message. You should get one from the allocator
itself.

> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!r) {
> +		dev_err(&pdev->dev, "failed to get mmio base\n");
> +		return NULL;
> +	}
> +
> +	lpwm->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(lpwm->clk)) {
> +		dev_err(&pdev->dev, "failed to get pwm clk\n");
> +		return NULL;
> +	}

"pwm" -> "PWM", "clk" -> "clock", please.

> +
> +	lpwm->chip.base = -1;
> +
> +	lpwm->regs = devm_request_and_ioremap(&pdev->dev, r);
> +	if (!lpwm->regs)

devm_ioremap_resource()? If so, it returns an ERR_PTR() encoded error
code on failure, so make sure to check with IS_ERR(lpwm->regs) instead.
Also you can get rid of the extra error checking after the call to
platform_get_resource() because devm_ioremap_resource() checks for it
already.

Furthermore the ordering of calls is somewhat unusual here. I'd prefer
platform_get_resource() followed by devm_ioremap_resource() directly.

> +		return NULL;
> +
> +	return lpwm;
> +}
> +
> +static const struct acpi_device_id pwm_lpss_acpi_match[] = {
> +	{ "80860F08", 0 },
> +	{ "80860F09", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match);
> +#else
> +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev)
> +{
> +	return NULL;
> +}
> +#endif

I think that #else is completely dead code since the driver depends on
ACPI and therefore CONFIG_ACPI will always be enabled when building this
driver.

> +static int pwm_lpss_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pwm_lpss_chip *lpwm;
> +	int ret;
> +
> +	lpwm = dev_get_platdata(dev);

struct pwm_lpss_chip is defined in this source file, how can anybody
else know what to fill platform_data with?

> +	if (!lpwm) {
> +		lpwm = pwm_lpss_acpi_get_pdata(pdev);
> +		if (!lpwm)
> +			return -ENODEV;
> +	}
[...]
> +static struct platform_driver pwm_lpss_driver = {
> +	.driver = {
> +		.name = "pwm-lpss",
> +		.acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match),

ACPI_PTR not needed since the table will always be there.

> +	},
> +	.probe = pwm_lpss_probe,
> +	.remove = pwm_lpss_remove,
> +};
> +module_platform_driver(pwm_lpss_driver);
> +
> +MODULE_DESCRIPTION("PWM driver for Intel LPSS");
> +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
> +MODULE_LICENSE("GPL");

The file header says GPL v2, but "GPL" here means "GPL v2 or later".

Thierry

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

Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[Mika Westerberg]

On Wed, Feb 26, 2014 at 03:38:23PM +0100, Thierry Reding wrote:
> On Tue, Jan 21, 2014 at 02:00:08AM +0800, Chew Chiau Ee wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> [...]
> > +/*
> > + * Intel Low Power Subsystem PWM controller driver
> > + *
> > + * Copyright (C) 2014, Intel Corporation
> > + * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
> > + * Author: Chew Kean Ho <kean.ho.chew@intel.com>
> > + * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > + * Author: Chew Chiau Ee <chiau.ee.chew@intel.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.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pwm.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define PWM				0x00000000
> > +#define PWM_ENABLE			BIT(31)
> > +#define PWM_SW_UPDATE			BIT(30)
> > +#define PWM_BASE_UNIT_SHIFT		8
> > +#define PWM_BASE_UNIT_MASK		0x00ffff00
> > +#define PWM_ON_TIME_DIV_MASK		0x000000ff
> 
> Does the documentation really call this "on time"? I've always only seen
> this called duty-cycle.

Yes, it's called like that in the documentation.

> 
> > +#define PWM_DIVISION_CORRECTION		0x2
> > +#define PWM_LIMIT			(0x8000 + PWM_DIVISION_CORRECTION)
> > +
> > +
> > +struct pwm_lpss_chip {
> > +	struct pwm_chip chip;
> > +	void __iomem *regs;
> > +	struct clk *clk;
> > +};
> > +
> > +static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
> > +{
> > +	return container_of(chip, struct pwm_lpss_chip, chip);
> > +}
> > +
> > +static void pwm_lpss_set_state(struct pwm_lpss_chip *lpwm, bool enable)
> > +{
> > +	u32 ctrl;
> > +
> > +	ctrl = readl(lpwm->regs + PWM);
> > +	if (enable)
> > +		ctrl |= PWM_ENABLE;
> > +	else
> > +		ctrl &= ~PWM_ENABLE;
> > +	writel(ctrl, lpwm->regs + PWM);
> 
> Nit: could use more blank lines around readl() and writel(), but I'm
> told that not everybody likes it that way, so if you absolutely must
> keep it this way, that's fine, too. =)
> 
> Also, is it really necessary to turn this into a function? It's only
> called in three places, where each call site would only require three
> lines. This function takes up 12 lines in total, so there's no real
> gain.

Good point.

> 
> > +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +	int duty_ns, int period_ns)
> 
> Align arguments on subsequent lines with those of the first line,
> please.

OK

> 
> > +{
> > +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> > +	u8 on_time_div;
> > +	unsigned long c = clk_get_rate(lpwm->clk);
> > +	unsigned long long base_unit, hz = 1000000000UL;
> 
> "hz" -> "freq"? "1000000000UL" -> "NSECS_PER_SEC"?

OK

> 
> > +static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> > +
> > +	clk_prepare_enable(lpwm->clk);
> 
> You need to check the return value of clk_prepare_enable().

Indeed. Will fix.

> 
> > +#ifdef CONFIG_ACPI
> > +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev)
> > +{
> > +	struct pwm_lpss_chip *lpwm;
> > +	struct resource *r;
> > +
> > +	lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL);
> > +	if (!lpwm) {
> > +		dev_err(&pdev->dev, "failed to allocate memory for platform data\n");
> 
> No need to print this message. You should get one from the allocator
> itself.

OK

> 
> > +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!r) {
> > +		dev_err(&pdev->dev, "failed to get mmio base\n");
> > +		return NULL;
> > +	}
> > +
> > +	lpwm->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(lpwm->clk)) {
> > +		dev_err(&pdev->dev, "failed to get pwm clk\n");
> > +		return NULL;
> > +	}
> 
> "pwm" -> "PWM", "clk" -> "clock", please.

OK

> 
> > +
> > +	lpwm->chip.base = -1;
> > +
> > +	lpwm->regs = devm_request_and_ioremap(&pdev->dev, r);
> > +	if (!lpwm->regs)
> 
> devm_ioremap_resource()? If so, it returns an ERR_PTR() encoded error
> code on failure, so make sure to check with IS_ERR(lpwm->regs) instead.
> Also you can get rid of the extra error checking after the call to
> platform_get_resource() because devm_ioremap_resource() checks for it
> already.
> 
> Furthermore the ordering of calls is somewhat unusual here. I'd prefer
> platform_get_resource() followed by devm_ioremap_resource() directly.

Yup, we will change that.

> 
> > +		return NULL;
> > +
> > +	return lpwm;
> > +}
> > +
> > +static const struct acpi_device_id pwm_lpss_acpi_match[] = {
> > +	{ "80860F08", 0 },
> > +	{ "80860F09", 0 },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match);
> > +#else
> > +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev)
> > +{
> > +	return NULL;
> > +}
> > +#endif
> 
> I think that #else is completely dead code since the driver depends on
> ACPI and therefore CONFIG_ACPI will always be enabled when building this
> driver.

We are getting PCI enumeration for this device as well but for now we can
drop the code and add it back later if needed.

> 
> > +static int pwm_lpss_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct pwm_lpss_chip *lpwm;
> > +	int ret;
> > +
> > +	lpwm = dev_get_platdata(dev);
> 
> struct pwm_lpss_chip is defined in this source file, how can anybody
> else know what to fill platform_data with?

Good point. Chiau Ee, do you recall why that thing is there in the first
place?

> > +	if (!lpwm) {
> > +		lpwm = pwm_lpss_acpi_get_pdata(pdev);
> > +		if (!lpwm)
> > +			return -ENODEV;
> > +	}
> [...]
> > +static struct platform_driver pwm_lpss_driver = {
> > +	.driver = {
> > +		.name = "pwm-lpss",
> > +		.acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match),
> 
> ACPI_PTR not needed since the table will always be there.

OK.

> 
> > +	},
> > +	.probe = pwm_lpss_probe,
> > +	.remove = pwm_lpss_remove,
> > +};
> > +module_platform_driver(pwm_lpss_driver);
> > +
> > +MODULE_DESCRIPTION("PWM driver for Intel LPSS");
> > +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
> > +MODULE_LICENSE("GPL");
> 
> The file header says GPL v2, but "GPL" here means "GPL v2 or later".

OK, we will change that to GPLv2.

Thanks a lot for your review! We will do the changes and submit v2.

RE: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[Chew, Chiau Ee]

> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: Thursday, February 27, 2014 5:02 PM
> To: Thierry Reding
> Cc: Chew, Chiau Ee; linux-pwm@vger.kernel.org; linux-kernel@vger.kernel.org;
> Chew, Kean Ho; Chang, Rebecca Swee Fun
> Subject: Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM
> 
> On Wed, Feb 26, 2014 at 03:38:23PM +0100, Thierry Reding wrote:
> > On Tue, Jan 21, 2014 at 02:00:08AM +0800, Chew Chiau Ee wrote:
> > [...]
> > > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> > [...]
> > > +/*
> > > + * Intel Low Power Subsystem PWM controller driver
> > > + *
> > > + * Copyright (C) 2014, Intel Corporation
> > > + * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > + * Author: Chew Kean Ho <kean.ho.chew@intel.com>
> > > + * Author: Chang Rebecca Swee Fun
> > > +<rebecca.swee.fun.chang@intel.com>
> > > + * Author: Chew Chiau Ee <chiau.ee.chew@intel.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.
> > > + */
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/device.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#define PWM				0x00000000
> > > +#define PWM_ENABLE			BIT(31)
> > > +#define PWM_SW_UPDATE			BIT(30)
> > > +#define PWM_BASE_UNIT_SHIFT		8
> > > +#define PWM_BASE_UNIT_MASK		0x00ffff00
> > > +#define PWM_ON_TIME_DIV_MASK		0x000000ff
> >
> > Does the documentation really call this "on time"? I've always only
> > seen this called duty-cycle.
> 
> Yes, it's called like that in the documentation.
> 
> >
> > > +#define PWM_DIVISION_CORRECTION		0x2
> > > +#define PWM_LIMIT			(0x8000 +
> PWM_DIVISION_CORRECTION)
> > > +
> > > +
> > > +struct pwm_lpss_chip {
> > > +	struct pwm_chip chip;
> > > +	void __iomem *regs;
> > > +	struct clk *clk;
> > > +};
> > > +
> > > +static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
> > > +{
> > > +	return container_of(chip, struct pwm_lpss_chip, chip); }
> > > +
> > > +static void pwm_lpss_set_state(struct pwm_lpss_chip *lpwm, bool
> > > +enable) {
> > > +	u32 ctrl;
> > > +
> > > +	ctrl = readl(lpwm->regs + PWM);
> > > +	if (enable)
> > > +		ctrl |= PWM_ENABLE;
> > > +	else
> > > +		ctrl &= ~PWM_ENABLE;
> > > +	writel(ctrl, lpwm->regs + PWM);
> >
> > Nit: could use more blank lines around readl() and writel(), but I'm
> > told that not everybody likes it that way, so if you absolutely must
> > keep it this way, that's fine, too. =)
> >
> > Also, is it really necessary to turn this into a function? It's only
> > called in three places, where each call site would only require three
> > lines. This function takes up 12 lines in total, so there's no real
> > gain.
> 
> Good point.
> 
> >
> > > +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > > +	int duty_ns, int period_ns)
> >
> > Align arguments on subsequent lines with those of the first line,
> > please.
> 
> OK
> 
> >
> > > +{
> > > +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> > > +	u8 on_time_div;
> > > +	unsigned long c = clk_get_rate(lpwm->clk);
> > > +	unsigned long long base_unit, hz = 1000000000UL;
> >
> > "hz" -> "freq"? "1000000000UL" -> "NSECS_PER_SEC"?
> 
> OK
> 
> >
> > > +static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device
> > > +*pwm) {
> > > +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> > > +
> > > +	clk_prepare_enable(lpwm->clk);
> >
> > You need to check the return value of clk_prepare_enable().
> 
> Indeed. Will fix.
> 
> >
> > > +#ifdef CONFIG_ACPI
> > > +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct
> > > +platform_device *pdev) {
> > > +	struct pwm_lpss_chip *lpwm;
> > > +	struct resource *r;
> > > +
> > > +	lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL);
> > > +	if (!lpwm) {
> > > +		dev_err(&pdev->dev, "failed to allocate memory for platform
> > > +data\n");
> >
> > No need to print this message. You should get one from the allocator
> > itself.
> 
> OK
> 
> >
> > > +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	if (!r) {
> > > +		dev_err(&pdev->dev, "failed to get mmio base\n");
> > > +		return NULL;
> > > +	}
> > > +
> > > +	lpwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > +	if (IS_ERR(lpwm->clk)) {
> > > +		dev_err(&pdev->dev, "failed to get pwm clk\n");
> > > +		return NULL;
> > > +	}
> >
> > "pwm" -> "PWM", "clk" -> "clock", please.
> 
> OK
> 
> >
> > > +
> > > +	lpwm->chip.base = -1;
> > > +
> > > +	lpwm->regs = devm_request_and_ioremap(&pdev->dev, r);
> > > +	if (!lpwm->regs)
> >
> > devm_ioremap_resource()? If so, it returns an ERR_PTR() encoded error
> > code on failure, so make sure to check with IS_ERR(lpwm->regs) instead.
> > Also you can get rid of the extra error checking after the call to
> > platform_get_resource() because devm_ioremap_resource() checks for it
> > already.
> >
> > Furthermore the ordering of calls is somewhat unusual here. I'd prefer
> > platform_get_resource() followed by devm_ioremap_resource() directly.
> 
> Yup, we will change that.
> 
> >
> > > +		return NULL;
> > > +
> > > +	return lpwm;
> > > +}
> > > +
> > > +static const struct acpi_device_id pwm_lpss_acpi_match[] = {
> > > +	{ "80860F08", 0 },
> > > +	{ "80860F09", 0 },
> > > +	{ },
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); #else struct
> > > +pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device
> > > +*pdev) {
> > > +	return NULL;
> > > +}
> > > +#endif
> >
> > I think that #else is completely dead code since the driver depends on
> > ACPI and therefore CONFIG_ACPI will always be enabled when building
> > this driver.
> 
> We are getting PCI enumeration for this device as well but for now we can drop
> the code and add it back later if needed.
> 
> >
> > > +static int pwm_lpss_probe(struct platform_device *pdev) {
> > > +	struct device *dev = &pdev->dev;
> > > +	struct pwm_lpss_chip *lpwm;
> > > +	int ret;
> > > +
> > > +	lpwm = dev_get_platdata(dev);
> >
> > struct pwm_lpss_chip is defined in this source file, how can anybody
> > else know what to fill platform_data with?
> 
> Good point. Chiau Ee, do you recall why that thing is there in the first place?

This is added because we would eventually want to have PCI enumeration support
added. I think we shall find a way to better enable both the ACPI enumeration and
PCI enumeration support.

> 
> > > +	if (!lpwm) {
> > > +		lpwm = pwm_lpss_acpi_get_pdata(pdev);
> > > +		if (!lpwm)
> > > +			return -ENODEV;
> > > +	}
> > [...]
> > > +static struct platform_driver pwm_lpss_driver = {
> > > +	.driver = {
> > > +		.name = "pwm-lpss",
> > > +		.acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match),
> >
> > ACPI_PTR not needed since the table will always be there.
> 
> OK.
> 
> >
> > > +	},
> > > +	.probe = pwm_lpss_probe,
> > > +	.remove = pwm_lpss_remove,
> > > +};
> > > +module_platform_driver(pwm_lpss_driver);
> > > +
> > > +MODULE_DESCRIPTION("PWM driver for Intel LPSS");
> > > +MODULE_AUTHOR("Mika Westerberg
> <mika.westerberg@linux.intel.com>");
> > > +MODULE_LICENSE("GPL");
> >
> > The file header says GPL v2, but "GPL" here means "GPL v2 or later".
> 
> OK, we will change that to GPLv2.
> 
> Thanks a lot for your review! We will do the changes and submit v2.

Ok. I will make the changes accordingly.

Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[Mika Westerberg]

On Thu, Feb 27, 2014 at 09:03:41AM +0000, Chew, Chiau Ee wrote:
> > > > +static int pwm_lpss_probe(struct platform_device *pdev) {
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct pwm_lpss_chip *lpwm;
> > > > +	int ret;
> > > > +
> > > > +	lpwm = dev_get_platdata(dev);
> > >
> > > struct pwm_lpss_chip is defined in this source file, how can anybody
> > > else know what to fill platform_data with?
> > 
> > Good point. Chiau Ee, do you recall why that thing is there in the first place?
> 
> This is added because we would eventually want to have PCI enumeration support
> added. I think we shall find a way to better enable both the ACPI enumeration and
> PCI enumeration support.

Agreed. For now, can you please drop the platform data thing from the
driver? Thanks.

> > > > +	if (!lpwm) {
> > > > +		lpwm = pwm_lpss_acpi_get_pdata(pdev);
> > > > +		if (!lpwm)
> > > > +			return -ENODEV;
> > > > +	}
> > > [...]
> > > > +static struct platform_driver pwm_lpss_driver = {
> > > > +	.driver = {
> > > > +		.name = "pwm-lpss",
> > > > +		.acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match),
> > >
> > > ACPI_PTR not needed since the table will always be there.
> > 
> > OK.
> > 
> > >
> > > > +	},
> > > > +	.probe = pwm_lpss_probe,
> > > > +	.remove = pwm_lpss_remove,
> > > > +};
> > > > +module_platform_driver(pwm_lpss_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("PWM driver for Intel LPSS");
> > > > +MODULE_AUTHOR("Mika Westerberg
> > <mika.westerberg@linux.intel.com>");
> > > > +MODULE_LICENSE("GPL");
> > >
> > > The file header says GPL v2, but "GPL" here means "GPL v2 or later".
> > 
> > OK, we will change that to GPLv2.
> > 
> > Thanks a lot for your review! We will do the changes and submit v2.
> 
> Ok. I will make the changes accordingly.

Thanks.

RE: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[Chew, Chiau Ee]

> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: Thursday, February 27, 2014 9:14 PM
> To: Chew, Chiau Ee
> Cc: Thierry Reding; linux-pwm@vger.kernel.org; linux-kernel@vger.kernel.org;
> Chew, Kean Ho; Chang, Rebecca Swee Fun
> Subject: Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM
> 
> On Thu, Feb 27, 2014 at 09:03:41AM +0000, Chew, Chiau Ee wrote:
> > > > > +static int pwm_lpss_probe(struct platform_device *pdev) {
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	struct pwm_lpss_chip *lpwm;
> > > > > +	int ret;
> > > > > +
> > > > > +	lpwm = dev_get_platdata(dev);
> > > >
> > > > struct pwm_lpss_chip is defined in this source file, how can
> > > > anybody else know what to fill platform_data with?
> > >
> > > Good point. Chiau Ee, do you recall why that thing is there in the first place?
> >
> > This is added because we would eventually want to have PCI enumeration
> > support added. I think we shall find a way to better enable both the
> > ACPI enumeration and PCI enumeration support.
> 
> Agreed. For now, can you please drop the platform data thing from the driver?
> Thanks.
> 

Ok. Noted.

> > > > > +	if (!lpwm) {
> > > > > +		lpwm = pwm_lpss_acpi_get_pdata(pdev);
> > > > > +		if (!lpwm)
> > > > > +			return -ENODEV;
> > > > > +	}
> > > > [...]
> > > > > +static struct platform_driver pwm_lpss_driver = {
> > > > > +	.driver = {
> > > > > +		.name = "pwm-lpss",
> > > > > +		.acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match),
> > > >
> > > > ACPI_PTR not needed since the table will always be there.
> > >
> > > OK.
> > >
> > > >
> > > > > +	},
> > > > > +	.probe = pwm_lpss_probe,
> > > > > +	.remove = pwm_lpss_remove,
> > > > > +};
> > > > > +module_platform_driver(pwm_lpss_driver);
> > > > > +
> > > > > +MODULE_DESCRIPTION("PWM driver for Intel LPSS");
> > > > > +MODULE_AUTHOR("Mika Westerberg
> > > <mika.westerberg@linux.intel.com>");
> > > > > +MODULE_LICENSE("GPL");
> > > >
> > > > The file header says GPL v2, but "GPL" here means "GPL v2 or later".
> > >
> > > OK, we will change that to GPLv2.
> > >
> > > Thanks a lot for your review! We will do the changes and submit v2.
> >
> > Ok. I will make the changes accordingly.
> 
> Thanks.