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

[PATCH v2 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!

Thierry, what should we do to get this series in? It was already more than 2
months of waiting for response from you. Two pings went to black hole.
So, Cc'ing to Linus directly this time, sorry.

Since v1:
- rebase on top of v4.10-rc2
- fix typo in commit message of patch 1
- add Mika's tag

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.11.0

[PATCH v2 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
derivative is used.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
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 72c0bce5a75c..8642feeb8abd 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.11.0

[PATCH v2 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.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
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 8642feeb8abd..ffa01ab907a6 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.11.0

[PATCH v2 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 3622f093490e..dfe0a2e8e4de 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 54433fc6d1a4..4ebcb5a86f78 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 ffa01ab907a6..e7d612e9df51 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 04766e0d41aa..88afc3086a2d 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.11.0

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

[Thierry Reding]

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

On Mon, Jan 02, 2017 at 11:16:46AM +0200, Andy Shevchenko wrote:
> 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(-)

Is there anything in particular that you think will need this change? It
looks to me more like churn than anything else. Moving away from the per
device struct to describe the particular instance seems to me like
removing flexibility that we might want at some point rather than any
real gain.

Thierry

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

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

[Mika Westerberg]

On Wed, Jan 18, 2017 at 12:11:09PM +0100, Thierry Reding wrote:
> On Mon, Jan 02, 2017 at 11:16:46AM +0200, Andy Shevchenko wrote:
> > 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(-)
> 
> Is there anything in particular that you think will need this change? It
> looks to me more like churn than anything else. Moving away from the per
> device struct to describe the particular instance seems to me like
> removing flexibility that we might want at some point rather than any
> real gain.

It simplifies the probe drivers for one. Since the core driver already
handles details of the specific SoC family, I don't think we need the
flexibility to be able to pass arbitrary platform data anyway.

No strong feelings, though. I'm fine either way :)

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

[Thierry Reding]

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

On Wed, Jan 18, 2017 at 03:01:38PM +0200, Mika Westerberg wrote:
> On Wed, Jan 18, 2017 at 12:11:09PM +0100, Thierry Reding wrote:
> > On Mon, Jan 02, 2017 at 11:16:46AM +0200, Andy Shevchenko wrote:
> > > 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(-)
> > 
> > Is there anything in particular that you think will need this change? It
> > looks to me more like churn than anything else. Moving away from the per
> > device struct to describe the particular instance seems to me like
> > removing flexibility that we might want at some point rather than any
> > real gain.
> 
> It simplifies the probe drivers for one. Since the core driver already
> handles details of the specific SoC family, I don't think we need the
> flexibility to be able to pass arbitrary platform data anyway.
> 
> No strong feelings, though. I'm fine either way :)

The current driver uses a strange inversion of the abstraction layer.
For one we have a "board info" structure that is supposed to describe
the variants of the hardware that exist. That data is in the core
driver, for reasons that I no longer remember, and then the PCI and
ACPI drivers reference those info structures depending on the type of
hardware they bind to. And worse, we now have to export symbols to
the PCI and ACPI drivers to make use of them.

I think this is the wrong way around. The core would ideally be unaware
of any particular variants and use only the struct pwm_lpss_boardinfo.
It would then be up to the ACPI and PCI drivers to provide the variants
they need.

Perhaps the only reason why the board info structures are in the core
driver is because the same variant exists as PCI and ACPI devices, so
putting them in the core removes potential duplication.

What I'm saying is that its wrong to have board specific bits in the
core driver. Duplicating the board info isn't a very attractive
alternative either, though, so it's not going to be elegant either way.

Thierry

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

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

[Mika Westerberg]

On Fri, Jan 20, 2017 at 12:00:51PM +0100, Thierry Reding wrote:
> On Wed, Jan 18, 2017 at 03:01:38PM +0200, Mika Westerberg wrote:
> > On Wed, Jan 18, 2017 at 12:11:09PM +0100, Thierry Reding wrote:
> > > On Mon, Jan 02, 2017 at 11:16:46AM +0200, Andy Shevchenko wrote:
> > > > 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(-)
> > > 
> > > Is there anything in particular that you think will need this change? It
> > > looks to me more like churn than anything else. Moving away from the per
> > > device struct to describe the particular instance seems to me like
> > > removing flexibility that we might want at some point rather than any
> > > real gain.
> > 
> > It simplifies the probe drivers for one. Since the core driver already
> > handles details of the specific SoC family, I don't think we need the
> > flexibility to be able to pass arbitrary platform data anyway.
> > 
> > No strong feelings, though. I'm fine either way :)
> 
> The current driver uses a strange inversion of the abstraction layer.
> For one we have a "board info" structure that is supposed to describe
> the variants of the hardware that exist. That data is in the core
> driver, for reasons that I no longer remember, and then the PCI and
> ACPI drivers reference those info structures depending on the type of
> hardware they bind to. And worse, we now have to export symbols to
> the PCI and ACPI drivers to make use of them.
> 
> I think this is the wrong way around. The core would ideally be unaware
> of any particular variants and use only the struct pwm_lpss_boardinfo.
> It would then be up to the ACPI and PCI drivers to provide the variants
> they need.
> 
> Perhaps the only reason why the board info structures are in the core
> driver is because the same variant exists as PCI and ACPI devices, so
> putting them in the core removes potential duplication.

Yes, I think that was the reason.

> What I'm saying is that its wrong to have board specific bits in the
> core driver. Duplicating the board info isn't a very attractive
> alternative either, though, so it's not going to be elegant either way.

Agreed.

I think we can drop this patch then and make another that moves the
exported information to both ACPI and PCI drivers (instead of exporting
it from the core driver). That adds some duplication but then keeps the
core driver clean from SoC specific quirks.

Does that work for you?

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

[Thierry Reding]

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

On Fri, Jan 20, 2017 at 01:15:29PM +0200, Mika Westerberg wrote:
> On Fri, Jan 20, 2017 at 12:00:51PM +0100, Thierry Reding wrote:
> > On Wed, Jan 18, 2017 at 03:01:38PM +0200, Mika Westerberg wrote:
> > > On Wed, Jan 18, 2017 at 12:11:09PM +0100, Thierry Reding wrote:
> > > > On Mon, Jan 02, 2017 at 11:16:46AM +0200, Andy Shevchenko wrote:
> > > > > 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(-)
> > > > 
> > > > Is there anything in particular that you think will need this change? It
> > > > looks to me more like churn than anything else. Moving away from the per
> > > > device struct to describe the particular instance seems to me like
> > > > removing flexibility that we might want at some point rather than any
> > > > real gain.
> > > 
> > > It simplifies the probe drivers for one. Since the core driver already
> > > handles details of the specific SoC family, I don't think we need the
> > > flexibility to be able to pass arbitrary platform data anyway.
> > > 
> > > No strong feelings, though. I'm fine either way :)
> > 
> > The current driver uses a strange inversion of the abstraction layer.
> > For one we have a "board info" structure that is supposed to describe
> > the variants of the hardware that exist. That data is in the core
> > driver, for reasons that I no longer remember, and then the PCI and
> > ACPI drivers reference those info structures depending on the type of
> > hardware they bind to. And worse, we now have to export symbols to
> > the PCI and ACPI drivers to make use of them.
> > 
> > I think this is the wrong way around. The core would ideally be unaware
> > of any particular variants and use only the struct pwm_lpss_boardinfo.
> > It would then be up to the ACPI and PCI drivers to provide the variants
> > they need.
> > 
> > Perhaps the only reason why the board info structures are in the core
> > driver is because the same variant exists as PCI and ACPI devices, so
> > putting them in the core removes potential duplication.
> 
> Yes, I think that was the reason.
> 
> > What I'm saying is that its wrong to have board specific bits in the
> > core driver. Duplicating the board info isn't a very attractive
> > alternative either, though, so it's not going to be elegant either way.
> 
> Agreed.
> 
> I think we can drop this patch then and make another that moves the
> exported information to both ACPI and PCI drivers (instead of exporting
> it from the core driver). That adds some duplication but then keeps the
> core driver clean from SoC specific quirks.
> 
> Does that work for you?

Yeah, that works for me. It's not ideal, but I think it's the lesser of
two evils.

Thierry

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

[PATCH v2 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.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
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 e7d612e9df51..7d3ac8204618 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.11.0

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

[Thierry Reding]

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

On Mon, Jan 02, 2017 at 11:16:47AM +0200, 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.
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 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 e7d612e9df51..7d3ac8204618 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);
> +	}

Would you mind doing another pass over this and inline the
pwm_lpss_enable(), pwm_lpss_disable() and pwm_lpss_config() functions
into pwm_lpss_apply()? Your current version effectively duplicates the
transitional helpers, but the goal should be to fully get rid of the
remainders of the legacy API.

Besides being much cleaner this also gets rid of slight inconsistencies
between the two APIs.

Thierry

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

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

[Andy Shevchenko]

On Wed, 2017-01-18 at 12:15 +0100, Thierry Reding wrote:
> On Mon, Jan 02, 2017 at 11:16:47AM +0200, 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.

> > +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);
> > +	}
> 
> Would you mind doing another pass over this and inline the
> pwm_lpss_enable(), pwm_lpss_disable() and pwm_lpss_config() functions
> into pwm_lpss_apply()? Your current version effectively duplicates the
> transitional helpers, but the goal should be to fully get rid of the
> remainders of the legacy API.

I don't see how inlining them helps. For me readability of code is more
important than names of the functions.

I can rename functions, but I would like to have them separate from the
->apply() callback.

Compiler inlines them during optimization.

> Besides being much cleaner this also gets rid of slight
> inconsistencies
> between the two APIs.

I don't see how One Big Function can be cleaner than split version. But
I give a try to see it on my side when you have settled with Mika's
patch.

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

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

[Thierry Reding]

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

On Thu, Jan 19, 2017 at 04:32:46PM +0200, Andy Shevchenko wrote:
> On Wed, 2017-01-18 at 12:15 +0100, Thierry Reding wrote:
> > On Mon, Jan 02, 2017 at 11:16:47AM +0200, 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.
> 
> > > +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);
> > > +	}
> > 
> > Would you mind doing another pass over this and inline the
> > pwm_lpss_enable(), pwm_lpss_disable() and pwm_lpss_config() functions
> > into pwm_lpss_apply()? Your current version effectively duplicates the
> > transitional helpers, but the goal should be to fully get rid of the
> > remainders of the legacy API.
> 
> I don't see how inlining them helps. For me readability of code is more
> important than names of the functions.
> 
> I can rename functions, but I would like to have them separate from the
> ->apply() callback.
> 
> Compiler inlines them during optimization.
> 
> > Besides being much cleaner this also gets rid of slight
> > inconsistencies
> > between the two APIs.
> 
> I don't see how One Big Function can be cleaner than split version. But
> I give a try to see it on my side when you have settled with Mika's
> patch.

It doesn't necessarily have to be one big function. You obviously need
to find the right balance. If the combined function is still readable,
there's no reason, in my opinion, to split it up. If it gets too large
I think a good split is to keep all of the register programming to be
within ->apply() and have a helper that computes the values to program
and that is called from ->apply().

What I want to avoid is people converting to atomic by mindlessly
duplicating the fallback path in pwm_apply_state().

The goal of the atomic API is that programming of the hardware becomes
atomic. So if you have any computations that may fail because of invalid
inputs or similar, then all of that should happen before any registers
are touched. That's difficult to do with the old enable/config/disable
callbacks, so I don't think we should be following it when converting.

Thierry

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

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

[Ilkka Koskinen]

On Mon, Jan 02, 2017 at 11:16:43AM +0200, 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, what should we do to get this series in? It was already more than 2
> months of waiting for response from you. Two pings went to black hole.
> So, Cc'ing to Linus directly this time, sorry.
> 
> Since v1:
> - rebase on top of v4.10-rc2
> - fix typo in commit message of patch 1
> - add Mika's tag
> 
> 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.11.0
> 

Tested-by: Ilkka Koskinen <ilkka.koskinen@intel.com>

Cheers, Ilkka