[PATCH v1 0/4] pwm: lpss: clean up series

[PATCH v1 0/4] pwm: lpss: clean up series

[Andy Shevchenko]

There are clean ups and switch to new atomic API (would be considered as an
improvement).

The series tested on various Intel platforms, such as Edison and Minnowboard
MAX.

And the part I like most is -2 lines statistics!

Andy Shevchenko (3):
  pwm: lpss: Avoid potential overflow of base_unit
  pwm: lpss: Allow duty cycle to be 0
  pwm: lpss: Switch to new atomic API

Mika Westerberg (1):
  pwm: lpss: Do not export board infos for different PWM types

 drivers/pwm/pwm-lpss-pci.c      |  20 +++----
 drivers/pwm/pwm-lpss-platform.c |  10 ++--
 drivers/pwm/pwm-lpss.c          | 120 +++++++++++++++++++++-------------------
 drivers/pwm/pwm-lpss.h          |  14 ++---
 4 files changed, 81 insertions(+), 83 deletions(-)

-- 
2.9.3

[PATCH v1 1/4] pwm: lpss: Avoid potential overflow of base_unit

[Andy Shevchenko]

The resolution of base_unit is derived from base_unit_bits and thus must be
equal to (2^base_unit_bits - 1). Otherwise frequency and therefore base_unit
might potentially overflow.

Prevent the above by substracting 1 in all cases where base_unit_bits or
derevative is used.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/pwm-lpss.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 72c0bce..8642fee 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -102,7 +102,7 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * The equation is:
 	 * base_unit = round(base_unit_range * freq / c)
 	 */
-	base_unit_range = BIT(lpwm->info->base_unit_bits);
+	base_unit_range = BIT(lpwm->info->base_unit_bits) - 1;
 	freq *= base_unit_range;
 
 	base_unit = DIV_ROUND_CLOSEST_ULL(freq, c);
@@ -117,8 +117,8 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	ctrl = pwm_lpss_read(pwm);
 	ctrl &= ~PWM_ON_TIME_DIV_MASK;
-	ctrl &= ~((base_unit_range - 1) << PWM_BASE_UNIT_SHIFT);
-	base_unit &= (base_unit_range - 1);
+	ctrl &= ~(base_unit_range << PWM_BASE_UNIT_SHIFT);
+	base_unit &= base_unit_range;
 	ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT;
 	ctrl |= on_time_div;
 	pwm_lpss_write(pwm, ctrl);
-- 
2.9.3

Re: [PATCH v1 1/4] pwm: lpss: Avoid potential overflow of base_unit

[Mika Westerberg]

On Mon, Oct 24, 2016 at 05:43:22PM +0300, Andy Shevchenko wrote:
> The resolution of base_unit is derived from base_unit_bits and thus must be
> equal to (2^base_unit_bits - 1). Otherwise frequency and therefore base_unit
> might potentially overflow.
> 
> Prevent the above by substracting 1 in all cases where base_unit_bits or
> derevative is used.
  ^^^^^^^^^^
  derivative

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH v1 2/4] pwm: lpss: Allow duty cycle to be 0

[Andy Shevchenko]

A duty cycle is represented by values [0..<period>] which reflects [0%..100%].
0% of the duty cycle means always off (logical "0") on output. Allow this in
the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/pwm-lpss.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 8642fee..ffa01ab 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -107,8 +107,6 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	base_unit = DIV_ROUND_CLOSEST_ULL(freq, c);
 
-	if (duty_ns <= 0)
-		duty_ns = 1;
 	on_time_div = 255ULL * duty_ns;
 	do_div(on_time_div, period_ns);
 	on_time_div = 255ULL - on_time_div;
-- 
2.9.3

Re: [PATCH v1 2/4] pwm: lpss: Allow duty cycle to be 0

[Mika Westerberg]

On Mon, Oct 24, 2016 at 05:43:23PM +0300, Andy Shevchenko wrote:
> A duty cycle is represented by values [0..<period>] which reflects [0%..100%].
> 0% of the duty cycle means always off (logical "0") on output. Allow this in
> the driver.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH v1 3/4] pwm: lpss: Do not export board infos for different PWM types

[Andy Shevchenko]

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

The PWM LPSS probe drivers just pass a pointer to the exported board info
structures to pwm_lpss_probe() based on device PCI or ACPI ID. Since the
core driver knows everything else except mapping between device ID and the
type, just pass the type with pwm_lpss_probe() and stop exporting the board
info structures.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pwm/pwm-lpss-pci.c      | 20 ++++++++---------
 drivers/pwm/pwm-lpss-platform.c | 10 ++++-----
 drivers/pwm/pwm-lpss.c          | 49 ++++++++++++++++++++++-------------------
 drivers/pwm/pwm-lpss.h          | 14 +++++-------
 4 files changed, 44 insertions(+), 49 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
index 3622f09..dfe0a2e 100644
--- a/drivers/pwm/pwm-lpss-pci.c
+++ b/drivers/pwm/pwm-lpss-pci.c
@@ -20,7 +20,6 @@
 static int pwm_lpss_probe_pci(struct pci_dev *pdev,
 			      const struct pci_device_id *id)
 {
-	const struct pwm_lpss_boardinfo *info;
 	struct pwm_lpss_chip *lpwm;
 	int err;
 
@@ -28,8 +27,7 @@ static int pwm_lpss_probe_pci(struct pci_dev *pdev,
 	if (err < 0)
 		return err;
 
-	info = (struct pwm_lpss_boardinfo *)id->driver_data;
-	lpwm = pwm_lpss_probe(&pdev->dev, &pdev->resource[0], info);
+	lpwm = pwm_lpss_probe(&pdev->dev, &pdev->resource[0], id->driver_data);
 	if (IS_ERR(lpwm))
 		return PTR_ERR(lpwm);
 
@@ -73,14 +71,14 @@ static const struct dev_pm_ops pwm_lpss_pci_pm = {
 };
 
 static const struct pci_device_id pwm_lpss_pci_ids[] = {
-	{ PCI_VDEVICE(INTEL, 0x0ac8), (unsigned long)&pwm_lpss_bxt_info},
-	{ PCI_VDEVICE(INTEL, 0x0f08), (unsigned long)&pwm_lpss_byt_info},
-	{ PCI_VDEVICE(INTEL, 0x0f09), (unsigned long)&pwm_lpss_byt_info},
-	{ PCI_VDEVICE(INTEL, 0x11a5), (unsigned long)&pwm_lpss_bxt_info},
-	{ PCI_VDEVICE(INTEL, 0x1ac8), (unsigned long)&pwm_lpss_bxt_info},
-	{ PCI_VDEVICE(INTEL, 0x2288), (unsigned long)&pwm_lpss_bsw_info},
-	{ PCI_VDEVICE(INTEL, 0x2289), (unsigned long)&pwm_lpss_bsw_info},
-	{ PCI_VDEVICE(INTEL, 0x5ac8), (unsigned long)&pwm_lpss_bxt_info},
+	{ PCI_VDEVICE(INTEL, 0x0ac8), PWM_LPSS_BXT },
+	{ PCI_VDEVICE(INTEL, 0x0f08), PWM_LPSS_BYT },
+	{ PCI_VDEVICE(INTEL, 0x0f09), PWM_LPSS_BYT },
+	{ PCI_VDEVICE(INTEL, 0x11a5), PWM_LPSS_BXT },
+	{ PCI_VDEVICE(INTEL, 0x1ac8), PWM_LPSS_BXT },
+	{ PCI_VDEVICE(INTEL, 0x2288), PWM_LPSS_BSW },
+	{ PCI_VDEVICE(INTEL, 0x2289), PWM_LPSS_BSW },
+	{ PCI_VDEVICE(INTEL, 0x5ac8), PWM_LPSS_BXT },
 	{ },
 };
 MODULE_DEVICE_TABLE(pci, pwm_lpss_pci_ids);
diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index 54433fc..4ebcb5a 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -20,7 +20,6 @@
 
 static int pwm_lpss_probe_platform(struct platform_device *pdev)
 {
-	const struct pwm_lpss_boardinfo *info;
 	const struct acpi_device_id *id;
 	struct pwm_lpss_chip *lpwm;
 	struct resource *r;
@@ -29,10 +28,9 @@ static int pwm_lpss_probe_platform(struct platform_device *pdev)
 	if (!id)
 		return -ENODEV;
 
-	info = (const struct pwm_lpss_boardinfo *)id->driver_data;
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
-	lpwm = pwm_lpss_probe(&pdev->dev, r, info);
+	lpwm = pwm_lpss_probe(&pdev->dev, r, id->driver_data);
 	if (IS_ERR(lpwm))
 		return PTR_ERR(lpwm);
 
@@ -53,9 +51,9 @@ static int pwm_lpss_remove_platform(struct platform_device *pdev)
 }
 
 static const struct acpi_device_id pwm_lpss_acpi_match[] = {
-	{ "80860F09", (unsigned long)&pwm_lpss_byt_info },
-	{ "80862288", (unsigned long)&pwm_lpss_bsw_info },
-	{ "80865AC8", (unsigned long)&pwm_lpss_bxt_info },
+	{ "80860F09", PWM_LPSS_BYT },
+	{ "80862288", PWM_LPSS_BSW },
+	{ "80865AC8", PWM_LPSS_BXT },
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match);
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index ffa01ab..e7d612e 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -37,29 +37,32 @@ struct pwm_lpss_chip {
 	const struct pwm_lpss_boardinfo *info;
 };
 
-/* BayTrail */
-const struct pwm_lpss_boardinfo pwm_lpss_byt_info = {
-	.clk_rate = 25000000,
-	.npwm = 1,
-	.base_unit_bits = 16,
+struct pwm_lpss_boardinfo {
+	unsigned long clk_rate;
+	unsigned int npwm;
+	unsigned long base_unit_bits;
 };
-EXPORT_SYMBOL_GPL(pwm_lpss_byt_info);
 
-/* Braswell */
-const struct pwm_lpss_boardinfo pwm_lpss_bsw_info = {
-	.clk_rate = 19200000,
-	.npwm = 1,
-	.base_unit_bits = 16,
+static const struct pwm_lpss_boardinfo pwm_lpss_types[] = {
+	/* Baytrail */
+	[PWM_LPSS_BYT] = {
+		.clk_rate = 25000000,
+		.npwm = 1,
+		.base_unit_bits = 16,
+	},
+	/* Braswell */
+	[PWM_LPSS_BSW] = {
+		.clk_rate = 19200000,
+		.npwm = 1,
+		.base_unit_bits = 16,
+	},
+	/* Broxton */
+	[PWM_LPSS_BXT] = {
+		.clk_rate = 19200000,
+		.npwm = 4,
+		.base_unit_bits = 22,
+	},
 };
-EXPORT_SYMBOL_GPL(pwm_lpss_bsw_info);
-
-/* Broxton */
-const struct pwm_lpss_boardinfo pwm_lpss_bxt_info = {
-	.clk_rate = 19200000,
-	.npwm = 4,
-	.base_unit_bits = 22,
-};
-EXPORT_SYMBOL_GPL(pwm_lpss_bxt_info);
 
 static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
 {
@@ -160,7 +163,7 @@ static const struct pwm_ops pwm_lpss_ops = {
 };
 
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
-				     const struct pwm_lpss_boardinfo *info)
+				     enum pwm_lpss_type type)
 {
 	struct pwm_lpss_chip *lpwm;
 	unsigned long c;
@@ -174,7 +177,7 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 	if (IS_ERR(lpwm->regs))
 		return ERR_CAST(lpwm->regs);
 
-	lpwm->info = info;
+	lpwm->info = &pwm_lpss_types[type];
 
 	c = lpwm->info->clk_rate;
 	if (!c)
@@ -183,7 +186,7 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 	lpwm->chip.dev = dev;
 	lpwm->chip.ops = &pwm_lpss_ops;
 	lpwm->chip.base = -1;
-	lpwm->chip.npwm = info->npwm;
+	lpwm->chip.npwm = lpwm->info->npwm;
 
 	ret = pwmchip_add(&lpwm->chip);
 	if (ret) {
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 04766e0..88afc30 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -18,18 +18,14 @@
 
 struct pwm_lpss_chip;
 
-struct pwm_lpss_boardinfo {
-	unsigned long clk_rate;
-	unsigned int npwm;
-	unsigned long base_unit_bits;
+enum pwm_lpss_type {
+	PWM_LPSS_BYT,
+	PWM_LPSS_BSW,
+	PWM_LPSS_BXT,
 };
 
-extern const struct pwm_lpss_boardinfo pwm_lpss_byt_info;
-extern const struct pwm_lpss_boardinfo pwm_lpss_bsw_info;
-extern const struct pwm_lpss_boardinfo pwm_lpss_bxt_info;
-
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
-				     const struct pwm_lpss_boardinfo *info);
+				     enum pwm_lpss_type type);
 int pwm_lpss_remove(struct pwm_lpss_chip *lpwm);
 
 #endif	/* __PWM_LPSS_H */
-- 
2.9.3

[PATCH v1 4/4] pwm: lpss: Switch to new atomic API

[Andy Shevchenko]

Instead of doing things separately, which is not so reliable on some platforms,
switch the driver to use new atomic API, i.e. ->apply() callback.

The change has been tested on Intel platforms such as Broxton, BayTrail, and
Merrifield.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/pwm-lpss.c | 63 +++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index e7d612e..7d3ac82 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -85,15 +85,20 @@ static inline void pwm_lpss_write(const struct pwm_device *pwm, u32 value)
 
 static void pwm_lpss_update(struct pwm_device *pwm)
 {
+	/*
+	 * Set a limit for busyloop since not all implementations correctly
+	 * clear PWM_SW_UPDATE bit (at least it's not visible on OS side).
+	 */
+	unsigned int count = 10;
+
 	pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
-	/* Give it some time to propagate */
-	usleep_range(10, 50);
+	while (pwm_lpss_read(pwm) & PWM_SW_UPDATE && --count)
+		usleep_range(10, 20);
 }
 
-static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
+static int pwm_lpss_config(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
 			   int duty_ns, int period_ns)
 {
-	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
 	unsigned long long on_time_div;
 	unsigned long c = lpwm->info->clk_rate, base_unit_range;
 	unsigned long long base_unit, freq = NSEC_PER_SEC;
@@ -114,8 +119,6 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	do_div(on_time_div, period_ns);
 	on_time_div = 255ULL - on_time_div;
 
-	pm_runtime_get_sync(chip->dev);
-
 	ctrl = pwm_lpss_read(pwm);
 	ctrl &= ~PWM_ON_TIME_DIV_MASK;
 	ctrl &= ~(base_unit_range << PWM_BASE_UNIT_SHIFT);
@@ -124,41 +127,43 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	ctrl |= on_time_div;
 	pwm_lpss_write(pwm, ctrl);
 
-	/*
-	 * If the PWM is already enabled we need to notify the hardware
-	 * about the change by setting PWM_SW_UPDATE.
-	 */
-	if (pwm_is_enabled(pwm))
-		pwm_lpss_update(pwm);
-
-	pm_runtime_put(chip->dev);
-
+	pwm_lpss_update(pwm);
 	return 0;
 }
 
-static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+static void pwm_lpss_enable(struct pwm_device *pwm)
 {
-	pm_runtime_get_sync(chip->dev);
-
-	/*
-	 * Hardware must first see PWM_SW_UPDATE before the PWM can be
-	 * enabled.
-	 */
-	pwm_lpss_update(pwm);
 	pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
-	return 0;
 }
 
-static void pwm_lpss_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+static void pwm_lpss_disable(struct pwm_device *pwm)
 {
 	pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
-	pm_runtime_put(chip->dev);
+}
+
+static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			  struct pwm_state *state)
+{
+	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
+
+	if (state->enabled) {
+		if (!pwm_is_enabled(pwm)) {
+			pm_runtime_get_sync(chip->dev);
+			pwm_lpss_config(lpwm, pwm, state->duty_cycle, state->period);
+			pwm_lpss_enable(pwm);
+		} else {
+			pwm_lpss_config(lpwm, pwm, state->duty_cycle, state->period);
+		}
+	} else if (pwm_is_enabled(pwm)) {
+		pwm_lpss_disable(pwm);
+		pm_runtime_put(chip->dev);
+	}
+
+	return 0;
 }
 
 static const struct pwm_ops pwm_lpss_ops = {
-	.config = pwm_lpss_config,
-	.enable = pwm_lpss_enable,
-	.disable = pwm_lpss_disable,
+	.apply = pwm_lpss_apply,
 	.owner = THIS_MODULE,
 };
 
-- 
2.9.3

Re: [PATCH v1 4/4] pwm: lpss: Switch to new atomic API

[Mika Westerberg]

On Mon, Oct 24, 2016 at 05:43:25PM +0300, Andy Shevchenko wrote:
> Instead of doing things separately, which is not so reliable on some platforms,
> switch the driver to use new atomic API, i.e. ->apply() callback.
> 
> The change has been tested on Intel platforms such as Broxton, BayTrail, and
> Merrifield.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Nice!

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 0/4] pwm: lpss: clean up series

[Andy Shevchenko]

On Mon, 2016-10-24 at 17:43 +0300, Andy Shevchenko wrote:
> There are clean ups and switch to new atomic API (would be considered
> as an
> improvement).
> 
> The series tested on various Intel platforms, such as Edison and
> Minnowboard
> MAX.
> 
> And the part I like most is -2 lines statistics!

Thierry, is there any chance to get it merged?

> 
> Andy Shevchenko (3):
>   pwm: lpss: Avoid potential overflow of base_unit
>   pwm: lpss: Allow duty cycle to be 0
>   pwm: lpss: Switch to new atomic API
> 
> Mika Westerberg (1):
>   pwm: lpss: Do not export board infos for different PWM types
> 
>  drivers/pwm/pwm-lpss-pci.c      |  20 +++----
>  drivers/pwm/pwm-lpss-platform.c |  10 ++--
>  drivers/pwm/pwm-lpss.c          | 120 +++++++++++++++++++++--------
> -----------
>  drivers/pwm/pwm-lpss.h          |  14 ++---
>  4 files changed, 81 insertions(+), 83 deletions(-)
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 0/4] pwm: lpss: clean up series

[Andy Shevchenko]

On Mon, 2016-10-24 at 17:43 +0300, Andy Shevchenko wrote:
> There are clean ups and switch to new atomic API (would be considered
> as an
> improvement).
> 
> The series tested on various Intel platforms, such as Edison and
> Minnowboard
> MAX.

It's already two months and no single comment, no movement somewhere.

What should we do to get this pushed?


> 
> And the part I like most is -2 lines statistics!
> 
> Andy Shevchenko (3):
>   pwm: lpss: Avoid potential overflow of base_unit
>   pwm: lpss: Allow duty cycle to be 0
>   pwm: lpss: Switch to new atomic API
> 
> Mika Westerberg (1):
>   pwm: lpss: Do not export board infos for different PWM types
> 
>  drivers/pwm/pwm-lpss-pci.c      |  20 +++----
>  drivers/pwm/pwm-lpss-platform.c |  10 ++--
>  drivers/pwm/pwm-lpss.c          | 120 +++++++++++++++++++++--------
> -----------
>  drivers/pwm/pwm-lpss.h          |  14 ++---
>  4 files changed, 81 insertions(+), 83 deletions(-)
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy