[PATCH v1 1/9] pwm: lpss: Deduplicate board info data structures

[PATCH v1 1/9] pwm: lpss: Deduplicate board info data structures

[Andy Shevchenko]

With help of __maybe_unused, that allows to avoid compilation warnings,
move the board info structures from the C files to the common header
and hence deduplicate configuration data.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/pwm-lpss-pci.c      | 29 -----------------------------
 drivers/pwm/pwm-lpss-platform.c | 23 -----------------------
 drivers/pwm/pwm-lpss.h          | 30 ++++++++++++++++++++++++++++++
 3 files changed, 30 insertions(+), 52 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..fcd80cca2f6d 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -15,29 +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)
 {
 	const struct pwm_lpss_boardinfo *info;
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 8b3476f25e06..3864f32c2487 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -33,6 +33,36 @@ struct pwm_lpss_boardinfo {
 	bool other_devices_aml_touches_pwm_regs;
 };
 
+/* BayTrail */
+static __maybe_unused const struct pwm_lpss_boardinfo pwm_lpss_byt_info = {
+	.clk_rate = 25000000,
+	.npwm = 1,
+	.base_unit_bits = 16,
+};
+
+/* Braswell */
+static __maybe_unused 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 __maybe_unused const struct pwm_lpss_boardinfo pwm_lpss_bxt_info = {
+	.clk_rate = 19200000,
+	.npwm = 4,
+	.base_unit_bits = 22,
+	.bypass = true,
+};
+
+/* Tangier */
+static __maybe_unused const struct pwm_lpss_boardinfo pwm_lpss_tng_info = {
+	.clk_rate = 19200000,
+	.npwm = 4,
+	.base_unit_bits = 22,
+};
+
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 				     const struct pwm_lpss_boardinfo *info);
 
-- 
2.35.1

[PATCH v1 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/

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, 3 insertions(+), 1 deletion(-)

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 fcd80cca2f6d..96fde1b2b967 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -87,4 +87,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 36d4e83e6b79..a82a57eb2482 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -250,7 +250,7 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 
 	return lpwm;
 }
-EXPORT_SYMBOL_GPL(pwm_lpss_probe);
+EXPORT_SYMBOL_NS_GPL(pwm_lpss_probe, PWM_LPSS);
 
 MODULE_DESCRIPTION("PWM driver for Intel LPSS");
 MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
-- 
2.35.1

[PATCH v1 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 96fde1b2b967..af57472f3ddc 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -20,16 +20,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 a82a57eb2482..44f9bb80611d 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -207,7 +207,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;
@@ -222,10 +222,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 3864f32c2487..5995b6b750a8 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -63,7 +63,7 @@ static __maybe_unused const struct pwm_lpss_boardinfo pwm_lpss_tng_info = {
 	.base_unit_bits = 22,
 };
 
-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 v1 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.

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 5995b6b750a8..832cb86996d7 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 v1 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 af57472f3ddc..beb707d67b99 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -7,27 +7,25 @@
  * 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"
 
 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 v1 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 v1 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 44f9bb80611d..276b7a3a34ff 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>
@@ -24,7 +25,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 v1 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 832cb86996d7..35e570067fc6 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 v1 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 ++---------------
 include/linux/platform_data/x86/pwm-lpss.h | 33 ++++++++++++++++++++++
 2 files changed, 35 insertions(+), 24 deletions(-)
 create mode 100644 include/linux/platform_data/x86/pwm-lpss.h

diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 35e570067fc6..41cb4b6246bb 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,23 +23,6 @@ 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;
-};
-
 /* BayTrail */
 static __maybe_unused const struct pwm_lpss_boardinfo pwm_lpss_byt_info = {
 	.clk_rate = 25000000,
@@ -72,7 +53,4 @@ static __maybe_unused const struct pwm_lpss_boardinfo pwm_lpss_tng_info = {
 	.base_unit_bits = 22,
 };
 
-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/include/linux/platform_data/x86/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
new file mode 100644
index 000000000000..296bd837ddbb
--- /dev/null
+++ b/include/linux/platform_data/x86/pwm-lpss.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Intel Low Power Subsystem PWM controller driver */
+
+#ifndef __PLATFORM_DATA_X86_PWM_LPSS_H
+#define __PLATFORM_DATA_X86_PWM_LPSS_H
+
+#include <linux/types.h>
+
+struct device;
+
+struct pwm_lpss_chip;
+
+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;
+};
+
+struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
+				     const struct pwm_lpss_boardinfo *info);
+
+#endif	/* __PLATFORM_DATA_X86_PWM_LPSS_H */
-- 
2.35.1

Re: [PATCH v1 1/9] pwm: lpss: Deduplicate board info data structures

[Uwe Kleine-König]

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

On Tue, Sep 06, 2022 at 10:57:27PM +0300, Andy Shevchenko wrote:
> With help of __maybe_unused, that allows to avoid compilation warnings,
> move the board info structures from the C files to the common header
> and hence deduplicate configuration data.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pwm/pwm-lpss-pci.c      | 29 -----------------------------
>  drivers/pwm/pwm-lpss-platform.c | 23 -----------------------
>  drivers/pwm/pwm-lpss.h          | 30 ++++++++++++++++++++++++++++++

Given that both the pci driver and the platform driver alread depend on
pwm-lpss.o, I'd prefer something like the patch below to really
deduplicate the data.

One thing to note is that the two pwm_lpss_bsw_info are not identical. I
didn't check how that is relevant. Did you check that?

Best regards
Uwe

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..e8b16b67bfd4 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -29,6 +29,39 @@
 /* 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,
+};
+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..918d2f177109 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_tng_info;
+extern const struct pwm_lpss_boardinfo pwm_lpss_bxt_info;
+extern const struct pwm_lpss_boardinfo pwm_lpss_bsw_info;
+extern const struct pwm_lpss_boardinfo pwm_lpss_byt_info;
+
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 				     const struct pwm_lpss_boardinfo *info);
 

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

[Uwe Kleine-König]

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

Hello Andy,

On Tue, Sep 06, 2022 at 10:57:28PM +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/
> 
> 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, 3 insertions(+), 1 deletion(-)
> 
> 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 fcd80cca2f6d..96fde1b2b967 100644
> --- a/drivers/pwm/pwm-lpss-platform.c
> +++ b/drivers/pwm/pwm-lpss-platform.c
> @@ -87,4 +87,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");

While it's not wrong to add the IMPORT_NS statement to each file, I'd
had added it to pwm-lpss.h. IMHO that makes sense as every includer of
that header needs that IMPORT_NS to actually use the symbols declared
there.

> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 36d4e83e6b79..a82a57eb2482 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -250,7 +250,7 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
>  
>  	return lpwm;
>  }
> -EXPORT_SYMBOL_GPL(pwm_lpss_probe);
> +EXPORT_SYMBOL_NS_GPL(pwm_lpss_probe, PWM_LPSS);

There is something possible with more magic:

	#define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS

which you only need once in pwm-lpss.c and then all exports use that
namespace. (And if you pick up my suggestion for patch 1 you also
benefit from that.)

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

[Uwe Kleine-König]

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

On Tue, Sep 06, 2022 at 10:57:30PM +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.
> 
> 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 5995b6b750a8..832cb86996d7 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;

the commit log doesn't explain the pwm_lpss_boardinfo part?!

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

[Andy Shevchenko]

On Wed, Sep 07, 2022 at 11:11:44AM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 06, 2022 at 10:57:28PM +0300, Andy Shevchenko wrote:

> >  MODULE_DESCRIPTION("PWM platform driver for Intel LPSS");
> >  MODULE_LICENSE("GPL v2");
> > +MODULE_IMPORT_NS(PWM_LPSS);
> >  MODULE_ALIAS("platform:pwm-lpss");
> 
> While it's not wrong to add the IMPORT_NS statement to each file, I'd
> had added it to pwm-lpss.h. IMHO that makes sense as every includer of
> that header needs that IMPORT_NS to actually use the symbols declared
> there.

If you have an optional dependency you may not need to include namespace
to avoid dragging it for peanuts.

...

> > -EXPORT_SYMBOL_GPL(pwm_lpss_probe);
> > +EXPORT_SYMBOL_NS_GPL(pwm_lpss_probe, PWM_LPSS);
> 
> There is something possible with more magic:

I know.

> 	#define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS
> 
> which you only need once in pwm-lpss.c and then all exports use that
> namespace. (And if you pick up my suggestion for patch 1 you also
> benefit from that.)

For a single export (even for a few of them) it's an overkill.

Taking above into consideration I don't think we need to alter
a proposed change.

Thanks for review!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Sep 07, 2022 at 11:13:35AM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 06, 2022 at 10:57:30PM +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.

...

> > +struct pwm_lpss_boardinfo;
> 
> the commit log doesn't explain the pwm_lpss_boardinfo part?!

Indeed, during one of amendment I rephrased it forgetting about
this small detail. (It was written in generic way to cover all
forward declarations)

I will add a word or two about this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/9] pwm: lpss: Deduplicate board info data structures

[Andy Shevchenko]

On Wed, Sep 07, 2022 at 11:04:12AM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 06, 2022 at 10:57:27PM +0300, Andy Shevchenko wrote:
> > With help of __maybe_unused, that allows to avoid compilation warnings,
> > move the board info structures from the C files to the common header
> > and hence deduplicate configuration data.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/pwm/pwm-lpss-pci.c      | 29 -----------------------------
> >  drivers/pwm/pwm-lpss-platform.c | 23 -----------------------
> >  drivers/pwm/pwm-lpss.h          | 30 ++++++++++++++++++++++++++++++
> 
> Given that both the pci driver and the platform driver alread depend on
> pwm-lpss.o, I'd prefer something like the patch below to really
> deduplicate the data.

Why not? I can use yours in v2. Can I get your SoB tag?

> One thing to note is that the two pwm_lpss_bsw_info are not identical. I
> didn't check how that is relevant. Did you check that?

Yes, 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. The no bug report is due to no PCI enabled device in the
wild, I think, and only reference boards can be tested, so nobody really cares
about PCI Braswell case.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Sep 07, 2022 at 05:21:53PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 07, 2022 at 11:11:44AM +0200, Uwe Kleine-König wrote:
> > On Tue, Sep 06, 2022 at 10:57:28PM +0300, Andy Shevchenko wrote:

...

> > > -EXPORT_SYMBOL_GPL(pwm_lpss_probe);
> > > +EXPORT_SYMBOL_NS_GPL(pwm_lpss_probe, PWM_LPSS);
> > 
> > There is something possible with more magic:
> > 	#define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS
> > 
> > which you only need once in pwm-lpss.c and then all exports use that
> > namespace. (And if you pick up my suggestion for patch 1 you also
> > benefit from that.)
> 
> For a single export (even for a few of them) it's an overkill.

Ah, you adding there 4 more. But still I think it's an overkill. It's so small
driver that duplicating namespace in each of the exported symbols is not an
issue, is it?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Sep 07, 2022 at 08:00:42PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 07, 2022 at 05:21:53PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 07, 2022 at 11:11:44AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Sep 06, 2022 at 10:57:28PM +0300, Andy Shevchenko wrote:

...

> > > > -EXPORT_SYMBOL_GPL(pwm_lpss_probe);
> > > > +EXPORT_SYMBOL_NS_GPL(pwm_lpss_probe, PWM_LPSS);
> > > 
> > > There is something possible with more magic:
> > > 	#define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS
> > > 
> > > which you only need once in pwm-lpss.c and then all exports use that
> > > namespace. (And if you pick up my suggestion for patch 1 you also
> > > benefit from that.)
> > 
> > For a single export (even for a few of them) it's an overkill.
> 
> Ah, you adding there 4 more. But still I think it's an overkill. It's so small
> driver that duplicating namespace in each of the exported symbols is not an
> issue, is it?

Okay, now looking at the patch organization (I forgot that I moved NS one to be
not the first one) your suggestion makes a point. We won't change the code we
just introduced.

That said, I would like to get your SoB or what you agree with to the patch 1
and I make this one as you suggested.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/9] pwm: lpss: Deduplicate board info data structures

[Uwe Kleine-König]

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

On Wed, Sep 07, 2022 at 05:27:22PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 07, 2022 at 11:04:12AM +0200, Uwe Kleine-König wrote:
> > On Tue, Sep 06, 2022 at 10:57:27PM +0300, Andy Shevchenko wrote:
> > > With help of __maybe_unused, that allows to avoid compilation warnings,
> > > move the board info structures from the C files to the common header
> > > and hence deduplicate configuration data.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  drivers/pwm/pwm-lpss-pci.c      | 29 -----------------------------
> > >  drivers/pwm/pwm-lpss-platform.c | 23 -----------------------
> > >  drivers/pwm/pwm-lpss.h          | 30 ++++++++++++++++++++++++++++++
> > 
> > Given that both the pci driver and the platform driver alread depend on
> > pwm-lpss.o, I'd prefer something like the patch below to really
> > deduplicate the data.
> 
> Why not? I can use yours in v2. Can I get your SoB tag?

Sure:

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

> > One thing to note is that the two pwm_lpss_bsw_info are not identical. I
> > didn't check how that is relevant. Did you check that?
> 
> Yes, 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. The no bug report is due to no PCI enabled device in the
> wild, I think, and only reference boards can be tested, so nobody really cares
> about PCI Braswell case.

I'm willing to believe that; please mention that in the commit log.

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 --]