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

[PATCH v2 0/9] 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 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 (8):
  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
  pwm: lpss: Allow other drivers to enable PWM LPSS

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                     | 22 ++++------
 include/linux/platform_data/x86/pwm-lpss.h | 33 +++++++++++++++
 5 files changed, 101 insertions(+), 88 deletions(-)
 create mode 100644 include/linux/platform_data/x86/pwm-lpss.h

-- 
2.35.1

[PATCH v2 1/9] 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>
---
 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 v2 2/9] 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>
---
 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 v2 3/9] 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>
---
 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 v2 4/9] 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, replace device.h with a forward declaration and add
missed struct pwm_lpss_boardinfo one.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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

[PATCH v2 5/9] 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>
---
 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 v2 6/9] 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>
---
 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 v2 7/9] 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>
---
 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 v2 8/9] 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>
---
 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

[PATCH v2 9/9] pwm: lpss: Allow other drivers to enable PWM LPSS

[Andy Shevchenko]

The PWM LPSS device can be embedded in another device.
In order to enable it, allow that drivers to probe
a corresponding device.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/pwm-lpss.h                        | 26 ++--------------
 .../linux/platform_data/x86}/pwm-lpss.h       | 30 ++++---------------
 2 files changed, 7 insertions(+), 49 deletions(-)
 copy {drivers/pwm => include/linux/platform_data/x86}/pwm-lpss.h (52%)

diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 0249c01befd5..fe32e336db8e 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -13,11 +13,9 @@
 #include <linux/pwm.h>
 #include <linux/types.h>
 
-#define MAX_PWMS			4
-
-struct device;
+#include <linux/platform_data/x86/pwm-lpss.h>
 
-struct pwm_lpss_boardinfo;
+#define MAX_PWMS			4
 
 struct pwm_lpss_chip {
 	struct pwm_chip chip;
@@ -25,29 +23,9 @@ struct pwm_lpss_chip {
 	const struct pwm_lpss_boardinfo *info;
 };
 
-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
-	 * messes with the PWM0 controllers state,
-	 */
-	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, void __iomem *base,
-				     const struct pwm_lpss_boardinfo *info);
-
 #endif	/* __PWM_LPSS_H */
diff --git a/drivers/pwm/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
similarity index 52%
copy from drivers/pwm/pwm-lpss.h
copy to include/linux/platform_data/x86/pwm-lpss.h
index 0249c01befd5..296bd837ddbb 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/include/linux/platform_data/x86/pwm-lpss.h
@@ -1,29 +1,14 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Intel Low Power Subsystem PWM controller driver
- *
- * Copyright (C) 2014, Intel Corporation
- *
- * Derived from the original pwm-lpss.c
- */
+/* Intel Low Power Subsystem PWM controller driver */
 
-#ifndef __PWM_LPSS_H
-#define __PWM_LPSS_H
+#ifndef __PLATFORM_DATA_X86_PWM_LPSS_H
+#define __PLATFORM_DATA_X86_PWM_LPSS_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;
-	const struct pwm_lpss_boardinfo *info;
-};
+struct pwm_lpss_chip;
 
 struct pwm_lpss_boardinfo {
 	unsigned long clk_rate;
@@ -42,12 +27,7 @@ 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, void __iomem *base,
 				     const struct pwm_lpss_boardinfo *info);
 
-#endif	/* __PWM_LPSS_H */
+#endif	/* __PLATFORM_DATA_X86_PWM_LPSS_H */
-- 
2.35.1

Re: [PATCH v2 0/9] pwm: lpss: Clean up and convert to a pure library

[Andy Shevchenko]

On Thu, Sep 08, 2022 at 04:56:49PM +0300, Andy Shevchenko wrote:
> 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.

What happened to the PWM subsystem maintenance again?
For weeks there is no reaction from the maintainer(s)... :-(
If there is a lag of maintaining, perhaps we should mark it
as Orphaned?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/9] pwm: lpss: Clean up and convert to a pure library

[Uwe Kleine-König]

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

Hello,

On Fri, Sep 23, 2022 at 08:03:39PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 08, 2022 at 04:56:49PM +0300, Andy Shevchenko wrote:
> > 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.
> 
> What happened to the PWM subsystem maintenance again?
> For weeks there is no reaction from the maintainer(s)... :-(
> If there is a lag of maintaining, perhaps we should mark it
> as Orphaned?

I thought I already acked most of the patches, will take another look.

Up to now it's Thierry who has the last word on things and applies
patches. I'm open to take a more active role.

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 v2 2/9] pwm: lpss: Move exported symbols to PWM_LPSS namespace

[Uwe Kleine-König]

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

Hello,

On Thu, Sep 08, 2022 at 04:56:51PM +0300, Andy Shevchenko wrote:
> 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>
> ---
>  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);

Each user of the lpss.h header needs that, right? Then the
MODULE_IMPORT_NS statement can go into the header, too.

Even without this change:

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

Thanks
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 v2 3/9] pwm: lpss: Move resource mapping to the glue drivers

[Uwe Kleine-König]

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

On Thu, Sep 08, 2022 at 04:56:52PM +0300, Andy Shevchenko wrote:
> 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>

LGTM,
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
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 v2 4/9] pwm: lpss: Include headers we are direct user of

[Uwe Kleine-König]

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

On Thu, Sep 08, 2022 at 04:56:53PM +0300, Andy Shevchenko wrote:
> For the sake of integrity, include headers we are direct user of.
> 
> While at it, replace device.h with a forward declaration and add
> missed struct pwm_lpss_boardinfo one.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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;

It's not clear to me how this is an improvment. Isn't it saner to
include <linux/device.h>?

> +struct pwm_lpss_boardinfo;

Why is this necessary? The struct is defined a few lines below the
context of this patch and I see no user that would benefit.

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 v2 5/9] pwm: lpss: Use device_get_match_data to get device data

[Uwe Kleine-König]

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

On Thu, Sep 08, 2022 at 04:56:54PM +0300, Andy Shevchenko wrote:
> 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>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

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 v2 6/9] pwm: lpss: Use DEFINE_RUNTIME_DEV_PM_OPS() and pm_ptr() macros

[Uwe Kleine-König]

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

On Thu, Sep 08, 2022 at 04:56:55PM +0300, Andy Shevchenko wrote:
> 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>

that was one of the patches I thought I already acked. I didn't check if
I'm confused or if you missed to add my Ack. Doesn't really matter to
me. So:

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

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 v2 7/9] pwm: lpss: Make use of bits.h macros for all masks

[Uwe Kleine-König]

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

On Thu, Sep 08, 2022 at 04:56:56PM +0300, Andy Shevchenko wrote:
> Make use of the GENMASK() (far less error-prone, far more concise).
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

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 v2 8/9] pwm: lpss: Add a comment to the bypass field

[Uwe Kleine-König]

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

On Thu, Sep 08, 2022 at 04:56:57PM +0300, Andy Shevchenko wrote:
> 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>

Looks good, added Hans (i.e. the author of b997e3edca4f) to Cc.

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

thanks
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 v2 9/9] pwm: lpss: Allow other drivers to enable PWM LPSS

[Uwe Kleine-König]

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

Hello Andy,

On Thu, Sep 08, 2022 at 04:56:58PM +0300, Andy Shevchenko wrote:
> The PWM LPSS device can be embedded in another device.
> In order to enable it, allow that drivers to probe
> a corresponding device.

There is no in-tree user of this. Do you plan to add one?
 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pwm/pwm-lpss.h                        | 26 ++--------------
>  .../linux/platform_data/x86}/pwm-lpss.h       | 30 ++++---------------
>  2 files changed, 7 insertions(+), 49 deletions(-)
>  copy {drivers/pwm => include/linux/platform_data/x86}/pwm-lpss.h (52%)
> 
> diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
> index 0249c01befd5..fe32e336db8e 100644
> --- a/drivers/pwm/pwm-lpss.h
> +++ b/drivers/pwm/pwm-lpss.h
> @@ -13,11 +13,9 @@
>  #include <linux/pwm.h>
>  #include <linux/types.h>
>  
> -#define MAX_PWMS			4
> -
> -struct device;
> +#include <linux/platform_data/x86/pwm-lpss.h>
>  
> -struct pwm_lpss_boardinfo;
> +#define MAX_PWMS			4

Side-note orthogonal to this patch series: IMHO this is a bad name for a
driver specific constant. Without a driver prefix you could easily
misjudge this as e.g. maximal number of PWMs a machine can have. This
should better be named LPSS_MAX_PWMS or similar.

>  
>  struct pwm_lpss_chip {
>  	struct pwm_chip chip;
> @@ -25,29 +23,9 @@ struct pwm_lpss_chip {
>  	const struct pwm_lpss_boardinfo *info;
>  };
>  
> -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
> -	 * messes with the PWM0 controllers state,
> -	 */
> -	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, void __iomem *base,
> -				     const struct pwm_lpss_boardinfo *info);
> -
>  #endif	/* __PWM_LPSS_H */
> diff --git a/drivers/pwm/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
> similarity index 52%
> copy from drivers/pwm/pwm-lpss.h
> copy to include/linux/platform_data/x86/pwm-lpss.h
> index 0249c01befd5..296bd837ddbb 100644
> --- a/drivers/pwm/pwm-lpss.h
> +++ b/include/linux/platform_data/x86/pwm-lpss.h
> @@ -1,29 +1,14 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Intel Low Power Subsystem PWM controller driver
> - *
> - * Copyright (C) 2014, Intel Corporation
> - *
> - * Derived from the original pwm-lpss.c
> - */
> +/* Intel Low Power Subsystem PWM controller driver */
>  
> -#ifndef __PWM_LPSS_H
> -#define __PWM_LPSS_H
> +#ifndef __PLATFORM_DATA_X86_PWM_LPSS_H
> +#define __PLATFORM_DATA_X86_PWM_LPSS_H
>  
> -#include <linux/pwm.h>
>  #include <linux/types.h>
>  
> -#define MAX_PWMS			4
> -
>  struct device;
>  
> -struct pwm_lpss_boardinfo;
> -

So the declaration you added before and I doubted is gone again.

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 v2 2/9] pwm: lpss: Move exported symbols to PWM_LPSS namespace

[Andy Shevchenko]

On Sat, Sep 24, 2022 at 11:59:45AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Sep 08, 2022 at 04:56:51PM +0300, Andy Shevchenko wrote:
> > 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>
> > ---
> >  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);
> 
> Each user of the lpss.h header needs that, right? Then the
> MODULE_IMPORT_NS statement can go into the header, too.

With the same answer as for v1: any user that might include the header for
the sake of data types will get the NS inclusion even if they don't need
that (yes, I don't think it's practical, but slightly better to make sure
that if one uses an API, one adds necessary NS inclusions; also note that
in case of stale header inclusion this again might bring unnecessary NS,
while the header should be removed -- with that being said, I think we
might need some kind of extended includecheck to see if the APIs and data
structures are actually used when a certain header is included).

> Even without this change:
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks!


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Sep 24, 2022 at 12:04:53PM +0200, Uwe Kleine-König wrote:
> On Thu, Sep 08, 2022 at 04:56:53PM +0300, Andy Shevchenko wrote:
> > For the sake of integrity, include headers we are direct user of.
> > 
> > While at it, replace device.h with a forward declaration and add
> > missed struct pwm_lpss_boardinfo one.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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;
> 
> It's not clear to me how this is an improvment. Isn't it saner to
> include <linux/device.h>?

The compilation time improvement. You don't need to include entire
train of unrelated stuff when compile something.

Moreover, the rule of thumb for the headers is avoid as much as possible
unrelated inclusions not only due to compilation time rising, but also
due to potential circular dependencies and increasing dependency hell
of headers. Believe me, we suffer a lot in the kernel due to this
(I have an example). Also you may check the Ingo's work of improving
headers breakage (APIs vs. implementation vs. data types, etc) to see
the achievement(s).

> > +struct pwm_lpss_boardinfo;
> 
> Why is this necessary? The struct is defined a few lines below the
> context of this patch and I see no user that would benefit.

This is clean way of how we program in C. We should forward declare
the types _before_ using them. Since this is a pointer, forward
declaration is enough.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/9] pwm: lpss: Use DEFINE_RUNTIME_DEV_PM_OPS() and pm_ptr() macros

[Andy Shevchenko]

On Sat, Sep 24, 2022 at 12:07:14PM +0200, Uwe Kleine-König wrote:
> On Thu, Sep 08, 2022 at 04:56:55PM +0300, Andy Shevchenko wrote:
> > 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>
> 
> that was one of the patches I thought I already acked. I didn't check if
> I'm confused or if you missed to add my Ack.

If so, sorry, and I think it might be because of wrong branch I took or misuse
of `git filter-branch --msg-filter`... But looking into [1] I do not see your
replies.

[1]: https://lore.kernel.org/linux-pwm/20220906195735.87361-1-andriy.shevchenko@linux.intel.com/

> Doesn't really matter to
> me. So:
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 9/9] pwm: lpss: Allow other drivers to enable PWM LPSS

[Andy Shevchenko]

On Sat, Sep 24, 2022 at 12:14:49PM +0200, Uwe Kleine-König wrote:
> On Thu, Sep 08, 2022 at 04:56:58PM +0300, Andy Shevchenko wrote:
> > The PWM LPSS device can be embedded in another device.
> > In order to enable it, allow that drivers to probe
> > a corresponding device.
> 
> There is no in-tree user of this. Do you plan to add one?

Yes.

...

> > +#define MAX_PWMS			4
> 
> Side-note orthogonal to this patch series: IMHO this is a bad name for a
> driver specific constant. Without a driver prefix you could easily
> misjudge this as e.g. maximal number of PWMs a machine can have. This
> should better be named LPSS_MAX_PWMS or similar.

Agree. But it was before my series. I can, of course, fix it at some point.

...

> > -struct pwm_lpss_boardinfo;
> 
> So the declaration you added before and I doubted is gone again.

I agree that it looks not nice in the same series, so I will drop this change
till I have a user. Then it will be clear that intention of the previous patch
is to make sure we don't abuse C programming language.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] pwm: lpss: Move exported symbols to PWM_LPSS namespace

[Uwe Kleine-König]

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

On Mon, Sep 26, 2022 at 12:43:47PM +0300, Andy Shevchenko wrote:
> On Sat, Sep 24, 2022 at 11:59:45AM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Thu, Sep 08, 2022 at 04:56:51PM +0300, Andy Shevchenko wrote:
> > > 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>
> > > ---
> > >  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);
> > 
> > Each user of the lpss.h header needs that, right? Then the
> > MODULE_IMPORT_NS statement can go into the header, too.
> 
> With the same answer as for v1: any user that might include the header for
> the sake of data types will get the NS inclusion even if they don't need
> that (yes, I don't think it's practical, but slightly better to make sure

I'm not sure I understand you correctly here. For some headers you
cannot assume that a file including the header also needs the namespace
macro, but for pwm-lpss.h that should be a safe assumption.

> that if one uses an API, one adds necessary NS inclusions; also note that
> in case of stale header inclusion this again might bring unnecessary NS,
> while the header should be removed -- with that being said, I think we
> might need some kind of extended includecheck to see if the APIs and data
> structures are actually used when a certain header is included).

+1 for a check about unused headers.

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 v2 2/9] pwm: lpss: Move exported symbols to PWM_LPSS namespace

[Andy Shevchenko]

On Mon, Sep 26, 2022 at 11:55:47AM +0200, Uwe Kleine-König wrote:
> On Mon, Sep 26, 2022 at 12:43:47PM +0300, Andy Shevchenko wrote:
> > On Sat, Sep 24, 2022 at 11:59:45AM +0200, Uwe Kleine-König wrote:
> > > On Thu, Sep 08, 2022 at 04:56:51PM +0300, Andy Shevchenko wrote:

...

> > > > +MODULE_IMPORT_NS(PWM_LPSS);
> > > 
> > > Each user of the lpss.h header needs that, right? Then the
> > > MODULE_IMPORT_NS statement can go into the header, too.
> > 
> > With the same answer as for v1: any user that might include the header for
> > the sake of data types will get the NS inclusion even if they don't need
> > that (yes, I don't think it's practical, but slightly better to make sure
> 
> I'm not sure I understand you correctly here. For some headers you
> cannot assume that a file including the header also needs the namespace
> macro, but for pwm-lpss.h that should be a safe assumption.

Yes, it's a safe assumption for _this_ case (as I pointed out above that
there is probably no practical to assume otherwise), in general it may be
not the case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/9] pwm: lpss: Clean up and convert to a pure library

[Hans de Goede]

Hi,

On 9/8/22 15:56, Andy Shevchenko wrote:
> 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 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 (8):
>   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
>   pwm: lpss: Allow other drivers to enable PWM LPSS
> 
> 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                     | 22 ++++------
>  include/linux/platform_data/x86/pwm-lpss.h | 33 +++++++++++++++
>  5 files changed, 101 insertions(+), 88 deletions(-)
>  create mode 100644 include/linux/platform_data/x86/pwm-lpss.h

Uwe, thank you for Cc-ing me.

Andy, thank you for the patches.

The entire series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

for the series.

Regards,

Hans