[PATCH v3 0/8] pwm: lpss: Clean up and convert to a pure library

[PATCH v3 0/8] pwm: lpss: Clean up and convert to a pure library

[Andy Shevchenko]

First of all, a set of cleanups and code deduplications (for better
maintenance) to the PWM LPSS driver.

Second, we may (re-)use the core part as a library in the future in
the devices that combine the same PWM IP in their address space. So
convert the core file to be a pure library which doesn't require any
special resource handling or alike.

Changelog v3:
- postponed last patch until we have a new user
- added tags (Uwe, Hans)
- expanded commit message on why forward declarations are preferred over
  full header inclusions

Changelog v2:
- replace patch 1 by Uwe's version (Uwe)
- update NS patch to have a default namespace defined (Uwe)
- describe all changes done in patch 4 (Uwe)

Andy Shevchenko (7):
  pwm: lpss: Move exported symbols to PWM_LPSS namespace
  pwm: lpss: Move resource mapping to the glue drivers
  pwm: lpss: Include headers we are direct user of
  pwm: lpss: Use device_get_match_data to get device data
  pwm: lpss: Use DEFINE_RUNTIME_DEV_PM_OPS() and pm_ptr() macros
  pwm: lpss: Make use of bits.h macros for all masks
  pwm: lpss: Add a comment to the bypass field

Uwe Kleine-König (1):
  pwm: lpss: Deduplicate board info data structures

 drivers/pwm/pwm-lpss-pci.c      | 48 ++++++++-------------------------
 drivers/pwm/pwm-lpss-platform.c | 40 +++++++--------------------
 drivers/pwm/pwm-lpss.c          | 46 ++++++++++++++++++++++++++-----
 drivers/pwm/pwm-lpss.h          | 18 +++++++++++--
 4 files changed, 77 insertions(+), 75 deletions(-)

-- 
2.35.1

[PATCH v3 1/8] pwm: lpss: Deduplicate board info data structures

[Andy Shevchenko]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Move the board info structures from the glue drivers to the
common library and hence deduplicate configuration data.

For the Intel Braswell case the ACPI version should be used.
Because switch to ACPI/PCI is done in BIOS while quite likely
the rest of AML code is the same, meaning similar issue might
be observed. There is no bug report due to no PCI enabled device
in the wild, Andy thinks, and only reference boards can be tested,
so nobody really cares about Intel Braswell PCI case.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/pwm-lpss-pci.c      | 29 ----------------------------
 drivers/pwm/pwm-lpss-platform.c | 22 ---------------------
 drivers/pwm/pwm-lpss.c          | 34 +++++++++++++++++++++++++++++++++
 drivers/pwm/pwm-lpss.h          |  5 +++++
 4 files changed, 39 insertions(+), 51 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
index c893ec3d2fb4..75b778e839b3 100644
--- a/drivers/pwm/pwm-lpss-pci.c
+++ b/drivers/pwm/pwm-lpss-pci.c
@@ -14,35 +14,6 @@
 
 #include "pwm-lpss.h"
 
-/* BayTrail */
-static const struct pwm_lpss_boardinfo pwm_lpss_byt_info = {
-	.clk_rate = 25000000,
-	.npwm = 1,
-	.base_unit_bits = 16,
-};
-
-/* Braswell */
-static const struct pwm_lpss_boardinfo pwm_lpss_bsw_info = {
-	.clk_rate = 19200000,
-	.npwm = 1,
-	.base_unit_bits = 16,
-};
-
-/* Broxton */
-static const struct pwm_lpss_boardinfo pwm_lpss_bxt_info = {
-	.clk_rate = 19200000,
-	.npwm = 4,
-	.base_unit_bits = 22,
-	.bypass = true,
-};
-
-/* Tangier */
-static const struct pwm_lpss_boardinfo pwm_lpss_tng_info = {
-	.clk_rate = 19200000,
-	.npwm = 4,
-	.base_unit_bits = 22,
-};
-
 static int pwm_lpss_probe_pci(struct pci_dev *pdev,
 			      const struct pci_device_id *id)
 {
diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index 928570430cef..834423c34f48 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -15,28 +15,6 @@
 
 #include "pwm-lpss.h"
 
-/* BayTrail */
-static const struct pwm_lpss_boardinfo pwm_lpss_byt_info = {
-	.clk_rate = 25000000,
-	.npwm = 1,
-	.base_unit_bits = 16,
-};
-
-/* Braswell */
-static const struct pwm_lpss_boardinfo pwm_lpss_bsw_info = {
-	.clk_rate = 19200000,
-	.npwm = 1,
-	.base_unit_bits = 16,
-	.other_devices_aml_touches_pwm_regs = true,
-};
-
-/* Broxton */
-static const struct pwm_lpss_boardinfo pwm_lpss_bxt_info = {
-	.clk_rate = 19200000,
-	.npwm = 4,
-	.base_unit_bits = 22,
-	.bypass = true,
-};
 
 static int pwm_lpss_probe_platform(struct platform_device *pdev)
 {
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 36d4e83e6b79..9537aefd254a 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -29,6 +29,40 @@
 /* Size of each PWM register space if multiple */
 #define PWM_SIZE			0x400
 
+/* BayTrail */
+const struct pwm_lpss_boardinfo pwm_lpss_byt_info = {
+	.clk_rate = 25000000,
+	.npwm = 1,
+	.base_unit_bits = 16,
+};
+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,
+	.other_devices_aml_touches_pwm_regs = true,
+};
+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,
+	.bypass = true,
+};
+EXPORT_SYMBOL_GPL(pwm_lpss_bxt_info);
+
+/* Tangier */
+const struct pwm_lpss_boardinfo pwm_lpss_tng_info = {
+	.clk_rate = 19200000,
+	.npwm = 4,
+	.base_unit_bits = 22,
+};
+EXPORT_SYMBOL_GPL(pwm_lpss_tng_info);
+
 static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
 {
 	return container_of(chip, struct pwm_lpss_chip, chip);
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 8b3476f25e06..9ea5b145a353 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -33,6 +33,11 @@ struct pwm_lpss_boardinfo {
 	bool other_devices_aml_touches_pwm_regs;
 };
 
+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;
+extern const struct pwm_lpss_boardinfo pwm_lpss_tng_info;
+
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 				     const struct pwm_lpss_boardinfo *info);
 
-- 
2.35.1

[PATCH v3 2/8] pwm: lpss: Move exported symbols to PWM_LPSS namespace

[Andy Shevchenko]

Avoid unnecessary pollution of the global symbol namespace by
moving library functions in to a specific namespace and import
that into the drivers that make use of the functions.

For more info: https://lwn.net/Articles/760045/

Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpss-pci.c      | 1 +
 drivers/pwm/pwm-lpss-platform.c | 1 +
 drivers/pwm/pwm-lpss.c          | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
index 75b778e839b3..9f2c666b95ec 100644
--- a/drivers/pwm/pwm-lpss-pci.c
+++ b/drivers/pwm/pwm-lpss-pci.c
@@ -92,3 +92,4 @@ module_pci_driver(pwm_lpss_driver_pci);
 
 MODULE_DESCRIPTION("PWM PCI driver for Intel LPSS");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(PWM_LPSS);
diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index 834423c34f48..65154c0abab1 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -88,4 +88,5 @@ module_platform_driver(pwm_lpss_driver_platform);
 
 MODULE_DESCRIPTION("PWM platform driver for Intel LPSS");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(PWM_LPSS);
 MODULE_ALIAS("platform:pwm-lpss");
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 9537aefd254a..74a296cb1af0 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -18,6 +18,8 @@
 #include <linux/pm_runtime.h>
 #include <linux/time.h>
 
+#define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS
+
 #include "pwm-lpss.h"
 
 #define PWM				0x00000000
-- 
2.35.1

[PATCH v3 3/8] pwm: lpss: Move resource mapping to the glue drivers

[Andy Shevchenko]

Move resource mapping to the glue drivers which helps
to transform pwm_lpss_probe() to pure library function
that may be used by others without need of specific
resource management.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpss-pci.c      | 6 +++++-
 drivers/pwm/pwm-lpss-platform.c | 9 ++++++---
 drivers/pwm/pwm-lpss.c          | 7 ++-----
 drivers/pwm/pwm-lpss.h          | 2 +-
 4 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
index 9f2c666b95ec..f3367e844e61 100644
--- a/drivers/pwm/pwm-lpss-pci.c
+++ b/drivers/pwm/pwm-lpss-pci.c
@@ -25,8 +25,12 @@ static int pwm_lpss_probe_pci(struct pci_dev *pdev,
 	if (err < 0)
 		return err;
 
+	err = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev));
+	if (err)
+		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, pcim_iomap_table(pdev)[0], info);
 	if (IS_ERR(lpwm))
 		return PTR_ERR(lpwm);
 
diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index 65154c0abab1..7bbbb7a9b578 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -21,16 +21,19 @@ 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;
+	void __iomem *base;
 
 	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
 	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);
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	lpwm = pwm_lpss_probe(&pdev->dev, base, info);
 	if (IS_ERR(lpwm))
 		return PTR_ERR(lpwm);
 
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 74a296cb1af0..a20915459809 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -243,7 +243,7 @@ static const struct pwm_ops pwm_lpss_ops = {
 	.owner = THIS_MODULE,
 };
 
-struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
+struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
 				     const struct pwm_lpss_boardinfo *info)
 {
 	struct pwm_lpss_chip *lpwm;
@@ -258,10 +258,7 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 	if (!lpwm)
 		return ERR_PTR(-ENOMEM);
 
-	lpwm->regs = devm_ioremap_resource(dev, r);
-	if (IS_ERR(lpwm->regs))
-		return ERR_CAST(lpwm->regs);
-
+	lpwm->regs = base;
 	lpwm->info = info;
 
 	c = lpwm->info->clk_rate;
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 9ea5b145a353..c344921b2cab 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -38,7 +38,7 @@ extern const struct pwm_lpss_boardinfo pwm_lpss_bsw_info;
 extern const struct pwm_lpss_boardinfo pwm_lpss_bxt_info;
 extern const struct pwm_lpss_boardinfo pwm_lpss_tng_info;
 
-struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
+struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
 				     const struct pwm_lpss_boardinfo *info);
 
 #endif	/* __PWM_LPSS_H */
-- 
2.35.1

[PATCH v3 4/8] pwm: lpss: Include headers we are direct user of

[Andy Shevchenko]

For the sake of integrity, include headers we are direct user of.

While at it, add missed struct pwm_lpss_boardinfo one and replace
device.h with a forward declaration. The latter improves compile
time due to reducing overhead of device.h parsing with entire train
of dependencies.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/pwm-lpss.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index c344921b2cab..839622964b2a 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -10,11 +10,15 @@
 #ifndef __PWM_LPSS_H
 #define __PWM_LPSS_H
 
-#include <linux/device.h>
 #include <linux/pwm.h>
+#include <linux/types.h>
 
 #define MAX_PWMS			4
 
+struct device;
+
+struct pwm_lpss_boardinfo;
+
 struct pwm_lpss_chip {
 	struct pwm_chip chip;
 	void __iomem *regs;
-- 
2.35.1

Re: [PATCH v3 4/8] pwm: lpss: Include headers we are direct user of

[Uwe Kleine-König]

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

Hello,

On Tue, Sep 27, 2022 at 05:47:19PM +0300, Andy Shevchenko wrote:
> For the sake of integrity, include headers we are direct user of.
> 
> While at it, add missed struct pwm_lpss_boardinfo one and replace
> device.h with a forward declaration. The latter improves compile
> time due to reducing overhead of device.h parsing with entire train
> of dependencies.

Hm, I copied the cmdline for the compiler from a V=1 build and only run
the compiler on drivers/pwm/pwm-lpss-pci.c.

With #include <device.h> I got:

	real	0m0.421s
	user	0m0.354s
	sys	0m0.066s

With struct device; I got:

	real	0m0.431s
	user	0m0.378s
	sys	0m0.052s

Are the numbers for you considerably different?

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pwm/pwm-lpss.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
> index c344921b2cab..839622964b2a 100644
> --- a/drivers/pwm/pwm-lpss.h
> +++ b/drivers/pwm/pwm-lpss.h
> @@ -10,11 +10,15 @@
>  #ifndef __PWM_LPSS_H
>  #define __PWM_LPSS_H
>  
> -#include <linux/device.h>
>  #include <linux/pwm.h>
> +#include <linux/types.h>
>  
>  #define MAX_PWMS			4
>  
> +struct device;
> +
> +struct pwm_lpss_boardinfo;

Hmm, I wonder why there is no compiler warning without that declaration.
At least in my builds. Do you see a warning? IMHO it's better to fix
that be swapping the order of struct pwm_lpss_chip and struct
pwm_lpss_boardinfo.

>  struct pwm_lpss_chip {
>  	struct pwm_chip chip;
>  	void __iomem *regs;

Best regards
Uwe

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

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

Re: [PATCH v3 4/8] pwm: lpss: Include headers we are direct user of

[Andy Shevchenko]

On Tue, Sep 27, 2022 at 05:10:53PM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 27, 2022 at 05:47:19PM +0300, Andy Shevchenko wrote:
> > For the sake of integrity, include headers we are direct user of.
> > 
> > While at it, add missed struct pwm_lpss_boardinfo one and replace
> > device.h with a forward declaration. The latter improves compile
> > time due to reducing overhead of device.h parsing with entire train
> > of dependencies.
> 
> Hm, I copied the cmdline for the compiler from a V=1 build and only run
> the compiler on drivers/pwm/pwm-lpss-pci.c.
> 
> With #include <device.h> I got:
> 
> 	real	0m0.421s
> 	user	0m0.354s
> 	sys	0m0.066s
> 
> With struct device; I got:
> 
> 	real	0m0.431s
> 	user	0m0.378s
> 	sys	0m0.052s
> 
> Are the numbers for you considerably different?

Why Ingo created thousands of patches to do something similar? Because for
a single user you won't see a big difference, but when amount of small pieces
are gathered together, you definitely will.

> > +struct device;

...

> > +struct pwm_lpss_boardinfo;
> 
> Hmm, I wonder why there is no compiler warning without that declaration.
> At least in my builds. Do you see a warning? IMHO it's better to fix
> that be swapping the order of struct pwm_lpss_chip and struct
> pwm_lpss_boardinfo.

Have I told about warning? It's a proper C programming style.
You don't have a warning because all pointers are considered to be the same,
but it is better style to explicitly point that out.

OTOH you may go and clean all the forward declarations of the data structures
in the kernel, but I believe the work wouldn't be appreciated much.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/8] pwm: lpss: Include headers we are direct user of

[Uwe Kleine-König]

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

On Tue, Sep 27, 2022 at 06:26:28PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 27, 2022 at 05:10:53PM +0200, Uwe Kleine-König wrote:
> > On Tue, Sep 27, 2022 at 05:47:19PM +0300, Andy Shevchenko wrote:
> > > For the sake of integrity, include headers we are direct user of.
> > > 
> > > While at it, add missed struct pwm_lpss_boardinfo one and replace
> > > device.h with a forward declaration. The latter improves compile
> > > time due to reducing overhead of device.h parsing with entire train
> > > of dependencies.
> > 
> > Hm, I copied the cmdline for the compiler from a V=1 build and only run
> > the compiler on drivers/pwm/pwm-lpss-pci.c.
> > 
> > With #include <device.h> I got:
> > 
> > 	real	0m0.421s
> > 	user	0m0.354s
> > 	sys	0m0.066s
> > 
> > With struct device; I got:
> > 
> > 	real	0m0.431s
> > 	user	0m0.378s
> > 	sys	0m0.052s
> > 
> > Are the numbers for you considerably different?
> 
> Why Ingo created thousands of patches to do something similar? Because for
> a single user you won't see a big difference, but when amount of small pieces
> are gathered together, you definitely will.

My doubt is that for me the effect of using struct device over #include
<device.h> is even negative (looking at real and user). Is it sys which
counts in the end?

> > > +struct device;
> 
> ...
> 
> > > +struct pwm_lpss_boardinfo;
> > 
> > Hmm, I wonder why there is no compiler warning without that declaration.
> > At least in my builds. Do you see a warning? IMHO it's better to fix
> > that be swapping the order of struct pwm_lpss_chip and struct
> > pwm_lpss_boardinfo.
> 
> Have I told about warning?

No, it's just me who expected there would be a warning if a pointer to
struct pwm_lpss_boardinfo is used before struct pwm_lpss_boardinfo is
defined (or declared).

Anyhow, I stand by my opinion that swapping the order of struct
pwm_lpss_chip and struct pwm_lpss_boardinfo is a saner fix.

> It's a proper C programming style.
> You don't have a warning because all pointers are considered to be the same,
> but it is better style to explicitly point that out.

Best regards
Uwe

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

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

Re: [PATCH v3 4/8] pwm: lpss: Include headers we are direct user of

[Andy Shevchenko]

On Tue, Sep 27, 2022 at 05:55:21PM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 27, 2022 at 06:26:28PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 27, 2022 at 05:10:53PM +0200, Uwe Kleine-König wrote:
> > > On Tue, Sep 27, 2022 at 05:47:19PM +0300, Andy Shevchenko wrote:
> > > > For the sake of integrity, include headers we are direct user of.
> > > > 
> > > > While at it, add missed struct pwm_lpss_boardinfo one and replace
> > > > device.h with a forward declaration. The latter improves compile
> > > > time due to reducing overhead of device.h parsing with entire train
> > > > of dependencies.
> > > 
> > > Hm, I copied the cmdline for the compiler from a V=1 build and only run
> > > the compiler on drivers/pwm/pwm-lpss-pci.c.
> > > 
> > > With #include <device.h> I got:
> > > 
> > > 	real	0m0.421s
> > > 	user	0m0.354s
> > > 	sys	0m0.066s
> > > 
> > > With struct device; I got:
> > > 
> > > 	real	0m0.431s
> > > 	user	0m0.378s
> > > 	sys	0m0.052s
> > > 
> > > Are the numbers for you considerably different?
> > 
> > Why Ingo created thousands of patches to do something similar? Because for
> > a single user you won't see a big difference, but when amount of small pieces
> > are gathered together, you definitely will.
> 
> My doubt is that for me the effect of using struct device over #include
> <device.h> is even negative (looking at real and user). Is it sys which
> counts in the end?

The main time required for the header inclusion is mmap():ing it (that's what
I believe preprocessor does) and parsing. In any case, testing on a high-speed
machine one file case is not correct (scientific) approach. Of course with
your measurements will be in the error range.

As I have learnt at university the very first question for the experiment
we should ask ourselves is "what exactly have we measured?"

I'm not sure any continuation of this makes sense if we haven't answered
to this. When I measured preprocessor speed up (not in this case, though),
I used ccache tool, because it makes more clear the time spend for C
preprocessor (by caching the compiler results), so 10 runs of that maybe
closer to the real world (hot cache which should have no effect on the
iteration-to-iteration time).

That said, if you want to NAK this, please do it explicitly. I'm not going
to waste my time on this simple change anymore.

> > > > +struct device;

...

> > > > +struct pwm_lpss_boardinfo;
> > > 
> > > Hmm, I wonder why there is no compiler warning without that declaration.
> > > At least in my builds. Do you see a warning? IMHO it's better to fix
> > > that be swapping the order of struct pwm_lpss_chip and struct
> > > pwm_lpss_boardinfo.
> > 
> > Have I told about warning?
> 
> No, it's just me who expected there would be a warning if a pointer to
> struct pwm_lpss_boardinfo is used before struct pwm_lpss_boardinfo is
> defined (or declared).
> 
> Anyhow, I stand by my opinion that swapping the order of struct
> pwm_lpss_chip and struct pwm_lpss_boardinfo is a saner fix.

I agree on this. But I expect a NAK, so simplest to me is to drop this patch
from the series.

> > It's a proper C programming style.
> > You don't have a warning because all pointers are considered to be the same,
> > but it is better style to explicitly point that out.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/8] pwm: lpss: Include headers we are direct user of

[Andy Shevchenko]

On Tue, Sep 27, 2022 at 07:18:36PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 27, 2022 at 05:55:21PM +0200, Uwe Kleine-König wrote:

...

> That said, if you want to NAK this, please do it explicitly. I'm not going
> to waste my time on this simple change anymore.

Just sent a v4 without this change, so I will not waste more time on this.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 5/8] pwm: lpss: Use device_get_match_data to get device data

[Andy Shevchenko]

device_get_match_data() in ACPI case calls similar to the
acpi_match_device(). We may simplify the code and make it
generic by replacing the latter with the former.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpss-platform.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index 7bbbb7a9b578..c48c6f2b2cd8 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -7,11 +7,12 @@
  * Derived from the original pwm-lpss.c
  */
 
-#include <linux/acpi.h>
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/property.h>
 
 #include "pwm-lpss.h"
 
@@ -19,16 +20,13 @@
 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;
 	void __iomem *base;
 
-	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
-	if (!id)
+	info = device_get_match_data(&pdev->dev);
+	if (!info)
 		return -ENODEV;
 
-	info = (const struct pwm_lpss_boardinfo *)id->driver_data;
-
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
-- 
2.35.1

[PATCH v3 6/8] pwm: lpss: Use DEFINE_RUNTIME_DEV_PM_OPS() and pm_ptr() macros

[Andy Shevchenko]

Using these new macros allows the compiler to remove the unused dev_pm_ops
structure and related functions if !CONFIG_PM without the need to mark
the functions __maybe_unused.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpss-pci.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
index f3367e844e61..98413d364338 100644
--- a/drivers/pwm/pwm-lpss-pci.c
+++ b/drivers/pwm/pwm-lpss-pci.c
@@ -48,7 +48,6 @@ static void pwm_lpss_remove_pci(struct pci_dev *pdev)
 	pm_runtime_get_sync(&pdev->dev);
 }
 
-#ifdef CONFIG_PM
 static int pwm_lpss_runtime_suspend_pci(struct device *dev)
 {
 	/*
@@ -62,12 +61,11 @@ static int pwm_lpss_runtime_resume_pci(struct device *dev)
 {
 	return 0;
 }
-#endif
 
-static const struct dev_pm_ops pwm_lpss_pci_pm = {
-	SET_RUNTIME_PM_OPS(pwm_lpss_runtime_suspend_pci,
-			   pwm_lpss_runtime_resume_pci, NULL)
-};
+static DEFINE_RUNTIME_DEV_PM_OPS(pwm_lpss_pci_pm,
+				 pwm_lpss_runtime_suspend_pci,
+				 pwm_lpss_runtime_resume_pci,
+				 NULL);
 
 static const struct pci_device_id pwm_lpss_pci_ids[] = {
 	{ PCI_VDEVICE(INTEL, 0x0ac8), (unsigned long)&pwm_lpss_bxt_info},
@@ -89,7 +87,7 @@ static struct pci_driver pwm_lpss_driver_pci = {
 	.probe = pwm_lpss_probe_pci,
 	.remove = pwm_lpss_remove_pci,
 	.driver = {
-		.pm = &pwm_lpss_pci_pm,
+		.pm = pm_ptr(&pwm_lpss_pci_pm),
 	},
 };
 module_pci_driver(pwm_lpss_driver_pci);
-- 
2.35.1

[PATCH v3 7/8] pwm: lpss: Make use of bits.h macros for all masks

[Andy Shevchenko]

Make use of the GENMASK() (far less error-prone, far more concise).

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpss.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index a20915459809..accdef5dd58e 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -10,6 +10,7 @@
  * Author: Alan Cox <alan@linux.intel.com>
  */
 
+#include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
@@ -26,7 +27,7 @@
 #define PWM_ENABLE			BIT(31)
 #define PWM_SW_UPDATE			BIT(30)
 #define PWM_BASE_UNIT_SHIFT		8
-#define PWM_ON_TIME_DIV_MASK		0x000000ff
+#define PWM_ON_TIME_DIV_MASK		GENMASK(7, 0)
 
 /* Size of each PWM register space if multiple */
 #define PWM_SIZE			0x400
-- 
2.35.1

[PATCH v3 8/8] pwm: lpss: Add a comment to the bypass field

[Andy Shevchenko]

Add a comment to the bypass field based on the commit b997e3edca4f
("pwm: lpss: Set enable-bit before waiting for update-bit
to go low").

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpss.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 839622964b2a..0249c01befd5 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -29,6 +29,11 @@ struct pwm_lpss_boardinfo {
 	unsigned long clk_rate;
 	unsigned int npwm;
 	unsigned long base_unit_bits;
+	/*
+	 * Some versions of the IP may stuck in the state machine if enable
+	 * bit is not set, and hence update bit will show busy status till
+	 * the reset. For the rest it may be otherwise.
+	 */
 	bool bypass;
 	/*
 	 * On some devices the _PS0/_PS3 AML code of the GPU (GFX0) device
-- 
2.35.1