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
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 <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
Make use of the GENMASK() (far less error-prone, far more concise).
Signed-off-by: Andy Shevchenko <[email protected]>
---
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 <[email protected]>
*/
+#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
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 <[email protected]>
---
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
From: Uwe Kleine-König <[email protected]>
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 <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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/ |
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 <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> 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 <[email protected]>
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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 <[email protected]>
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 <[email protected]>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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 <[email protected]>
Acked-by: Uwe Kleine-K?nig <[email protected]>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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 <[email protected]>
Acked-by: Uwe Kleine-K?nig <[email protected]>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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 <[email protected]>
LGTM,
Acked-by: Uwe Kleine-K?nig <[email protected]>
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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 <[email protected]>
> ---
> 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/ |
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
>
> 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/[email protected]/
> Doesn't really matter to
> me. So:
>
> Acked-by: Uwe Kleine-K?nig <[email protected]>
Thanks!
--
With Best Regards,
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
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 <[email protected]>
> > > Signed-off-by: Andy Shevchenko <[email protected]>
> > > ---
> > > 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/ |
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 <[email protected]>
> > Signed-off-by: Andy Shevchenko <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks!
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
for the series.
Regards,
Hans