[PATCH v4 0/2] pwm: lpss: split driver to core and probe drivers

[PATCH v4 0/2] pwm: lpss: split driver to core and probe drivers

[Andy Shevchenko]

This small series makes the architecture of pwm-lpss driver cleaner.

Since v3:
- rebased on top of recent linux-next
- rebased on top of Alan's patch for Braswell

Since v2:
- fix one more typo in Kconfig (tested with one line removed in my config now)

Since v1:
- hide struct definition in the core part
- rename pwm-lpss-plat to pwm-lpss-platform
- fix Kconfig dependencies and other typos
- add Mika's Reviewed-by tag
- fix spelling in the commit messages

Andy Shevchenko (2):
  pwm: lpss: properly split driver to parts
  pwm: lpss: pci: move to use pcim_enable_device()

 drivers/pwm/Kconfig             |  21 ++++++-
 drivers/pwm/Makefile            |   2 +
 drivers/pwm/pwm-lpss-pci.c      |  65 +++++++++++++++++++
 drivers/pwm/pwm-lpss-platform.c |  69 ++++++++++++++++++++
 drivers/pwm/pwm-lpss.c          | 136 +++-------------------------------------
 drivers/pwm/pwm-lpss.h          |  32 ++++++++++
 6 files changed, 198 insertions(+), 127 deletions(-)
 create mode 100644 drivers/pwm/pwm-lpss-pci.c
 create mode 100644 drivers/pwm/pwm-lpss-platform.c
 create mode 100644 drivers/pwm/pwm-lpss.h

-- 
2.1.0

[PATCH v4 1/2] pwm: lpss: properly split driver to parts

[Andy Shevchenko]

The driver consists core, PCI, and platform parts. It would better to split
them into separate files.

The platform driver is now called pwm-lpss-platform. Thus, prevously set
CONFIG_PWM_LPSS=m is not enough to build it. But we are on the safe side since
it seems no one from outside Intel is using it for now.

While here, move to use macros module_pci_driver() and
module_platform_driver().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pwm/Kconfig             |  21 ++++++-
 drivers/pwm/Makefile            |   2 +
 drivers/pwm/pwm-lpss-pci.c      |  66 +++++++++++++++++++
 drivers/pwm/pwm-lpss-platform.c |  69 ++++++++++++++++++++
 drivers/pwm/pwm-lpss.c          | 136 +++-------------------------------------
 drivers/pwm/pwm-lpss.h          |  32 ++++++++++
 6 files changed, 199 insertions(+), 127 deletions(-)
 create mode 100644 drivers/pwm/pwm-lpss-pci.c
 create mode 100644 drivers/pwm/pwm-lpss-platform.c
 create mode 100644 drivers/pwm/pwm-lpss.h

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b800783..b2539d7 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -149,7 +149,6 @@ config PWM_LPC32XX
 
 config PWM_LPSS
 	tristate "Intel LPSS PWM support"
-	depends on ACPI
 	help
 	  Generic PWM framework driver for Intel Low Power Subsystem PWM
 	  controller.
@@ -157,6 +156,26 @@ config PWM_LPSS
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-lpss.
 
+config PWM_LPSS_PCI
+	tristate "Intel LPSS PWM PCI driver"
+	depends on PCI
+	select PWM_LPSS
+	help
+	  The PCI driver for Intel Low Power Subsystem PWM controller.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-lpss-pci.
+
+config PWM_LPSS_PLATFORM
+	tristate "Intel LPSS PWM platform driver"
+	depends on ACPI
+	select PWM_LPSS
+	help
+	  The platform driver for Intel Low Power Subsystem PWM controller.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-lpss-platform.
+
 config PWM_MXS
 	tristate "Freescale MXS PWM support"
 	depends on ARCH_MXS && OF
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index f8c577d..c458606 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -13,6 +13,8 @@ obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
+obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
+obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
new file mode 100644
index 0000000..12515f3
--- /dev/null
+++ b/drivers/pwm/pwm-lpss-pci.c
@@ -0,0 +1,66 @@
+/*
+ * Intel Low Power Subsystem PWM controller PCI driver
+ *
+ * Copyright (C) 2014, Intel Corporation
+ *
+ * Derived from the original pwm-lpss.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+
+#include "pwm-lpss.h"
+
+static int pwm_lpss_probe_pci(struct pci_dev *pdev,
+			      const struct pci_device_id *id)
+{
+	const struct pwm_lpss_boardinfo *info;
+	struct pwm_lpss_chip *lpwm;
+	int err;
+
+	err = pci_enable_device(pdev);
+	if (err < 0)
+		return err;
+
+	info = (struct pwm_lpss_boardinfo *)id->driver_data;
+	lpwm = pwm_lpss_probe(&pdev->dev, &pdev->resource[0], info);
+	if (IS_ERR(lpwm))
+		return PTR_ERR(lpwm);
+
+	pci_set_drvdata(pdev, lpwm);
+	return 0;
+}
+
+static void pwm_lpss_remove_pci(struct pci_dev *pdev)
+{
+	struct pwm_lpss_chip *lpwm = pci_get_drvdata(pdev);
+
+	pwm_lpss_remove(lpwm);
+	pci_disable_device(pdev);
+}
+
+static struct pci_device_id pwm_lpss_pci_ids[] = {
+	{ PCI_VDEVICE(INTEL, 0x0f08), (unsigned long)&pwm_lpss_byt_info},
+	{ PCI_VDEVICE(INTEL, 0x0f09), (unsigned long)&pwm_lpss_byt_info},
+	{ PCI_VDEVICE(INTEL, 0x2288), (unsigned long)&pwm_lpss_bsw_info},
+	{ PCI_VDEVICE(INTEL, 0x2289), (unsigned long)&pwm_lpss_bsw_info},
+	{ },
+};
+MODULE_DEVICE_TABLE(pci, pwm_lpss_pci_ids);
+
+static struct pci_driver pwm_lpss_driver_pci = {
+	.name = "pwm-lpss",
+	.id_table = pwm_lpss_pci_ids,
+	.probe = pwm_lpss_probe_pci,
+	.remove = pwm_lpss_remove_pci,
+};
+
+module_pci_driver(pwm_lpss_driver_pci);
+
+MODULE_DESCRIPTION("PWM PCI driver for Intel LPSS");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
new file mode 100644
index 0000000..9424493
--- /dev/null
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -0,0 +1,69 @@
+/*
+ * Intel Low Power Subsystem PWM controller driver
+ *
+ * Copyright (C) 2014, Intel Corporation
+ *
+ * Derived from the original pwm-lpss.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/acpi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.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;
+	struct resource *r;
+
+	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
+	if (!id)
+		return -ENODEV;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	info = (struct pwm_lpss_boardinfo *)id->driver_data;
+	lpwm = pwm_lpss_probe(&pdev->dev, r, info);
+	if (IS_ERR(lpwm))
+		return PTR_ERR(lpwm);
+
+	platform_set_drvdata(pdev, lpwm);
+	return 0;
+}
+
+static int pwm_lpss_remove_platform(struct platform_device *pdev)
+{
+	struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev);
+
+	return pwm_lpss_remove(lpwm);
+}
+
+static const struct acpi_device_id pwm_lpss_acpi_match[] = {
+	{ "80860F09", (unsigned long)&pwm_lpss_byt_info },
+	{ "80862288", (unsigned long)&pwm_lpss_bsw_info },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match);
+
+static struct platform_driver pwm_lpss_driver_platform = {
+	.driver = {
+		.name = "pwm-lpss",
+		.acpi_match_table = pwm_lpss_acpi_match,
+	},
+	.probe = pwm_lpss_probe_platform,
+	.remove = pwm_lpss_remove_platform,
+};
+
+module_platform_driver(pwm_lpss_driver_platform);
+
+MODULE_DESCRIPTION("PWM platform driver for Intel LPSS");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:pwm-lpss");
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index d04eee7..ce9bf14 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -13,15 +13,10 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/acpi.h>
-#include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/pwm.h>
-#include <linux/platform_device.h>
-#include <linux/pci.h>
 
-static int pci_drv, plat_drv;	/* So we know which drivers registered */
+#include "pwm-lpss.h"
 
 #define PWM				0x00000000
 #define PWM_ENABLE			BIT(31)
@@ -39,19 +34,17 @@ struct pwm_lpss_chip {
 	unsigned long clk_rate;
 };
 
-struct pwm_lpss_boardinfo {
-	unsigned long clk_rate;
-};
-
 /* BayTrail */
-static const struct pwm_lpss_boardinfo byt_info = {
+const struct pwm_lpss_boardinfo pwm_lpss_byt_info = {
 	25000000
 };
+EXPORT_SYMBOL_GPL(pwm_lpss_byt_info);
 
 /* Braswell */
-static const struct pwm_lpss_boardinfo bsw_info = {
+const struct pwm_lpss_boardinfo pwm_lpss_bsw_info = {
 	19200000
 };
+EXPORT_SYMBOL_GPL(pwm_lpss_bsw_info);
 
 static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
 {
@@ -123,9 +116,8 @@ static const struct pwm_ops pwm_lpss_ops = {
 	.owner = THIS_MODULE,
 };
 
-static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
-					    struct resource *r,
-					    const struct pwm_lpss_boardinfo *info)
+struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
+				     const struct pwm_lpss_boardinfo *info)
 {
 	struct pwm_lpss_chip *lpwm;
 	int ret;
@@ -152,8 +144,9 @@ static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
 
 	return lpwm;
 }
+EXPORT_SYMBOL_GPL(pwm_lpss_probe);
 
-static int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
+int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
 {
 	u32 ctrl;
 
@@ -162,117 +155,8 @@ static int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
 
 	return pwmchip_remove(&lpwm->chip);
 }
-
-static int pwm_lpss_probe_pci(struct pci_dev *pdev,
-			      const struct pci_device_id *id)
-{
-	const struct pwm_lpss_boardinfo *info;
-	struct pwm_lpss_chip *lpwm;
-	int err;
-
-	err = pci_enable_device(pdev);
-	if (err < 0)
-		return err;
-
-	info = (struct pwm_lpss_boardinfo *)id->driver_data;
-	lpwm = pwm_lpss_probe(&pdev->dev, &pdev->resource[0], info);
-	if (IS_ERR(lpwm))
-		return PTR_ERR(lpwm);
-
-	pci_set_drvdata(pdev, lpwm);
-	return 0;
-}
-
-static void pwm_lpss_remove_pci(struct pci_dev *pdev)
-{
-	struct pwm_lpss_chip *lpwm = pci_get_drvdata(pdev);
-
-	pwm_lpss_remove(lpwm);
-	pci_disable_device(pdev);
-}
-
-static struct pci_device_id pwm_lpss_pci_ids[] = {
-	{ PCI_VDEVICE(INTEL, 0x0f08), (unsigned long)&byt_info},
-	{ PCI_VDEVICE(INTEL, 0x0f09), (unsigned long)&byt_info},
-	{ PCI_VDEVICE(INTEL, 0x2288), (unsigned long)&bsw_info},
-	{ PCI_VDEVICE(INTEL, 0x2289), (unsigned long)&bsw_info},
-	{ },
-};
-MODULE_DEVICE_TABLE(pci, pwm_lpss_pci_ids);
-
-static struct pci_driver pwm_lpss_driver_pci = {
-	.name = "pwm-lpss",
-	.id_table = pwm_lpss_pci_ids,
-	.probe = pwm_lpss_probe_pci,
-	.remove = pwm_lpss_remove_pci,
-};
-
-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;
-
-	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
-	if (!id)
-		return -ENODEV;
-
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
-	info = (struct pwm_lpss_boardinfo *)id->driver_data;
-	lpwm = pwm_lpss_probe(&pdev->dev, r, info);
-	if (IS_ERR(lpwm))
-		return PTR_ERR(lpwm);
-
-	platform_set_drvdata(pdev, lpwm);
-	return 0;
-}
-
-static int pwm_lpss_remove_platform(struct platform_device *pdev)
-{
-	struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev);
-
-	return pwm_lpss_remove(lpwm);
-}
-
-static const struct acpi_device_id pwm_lpss_acpi_match[] = {
-	{ "80860F09", (unsigned long)&byt_info },
-	{ "80862288", (unsigned long)&bsw_info },
-	{ },
-};
-MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match);
-
-static struct platform_driver pwm_lpss_driver_platform = {
-	.driver = {
-		.name = "pwm-lpss",
-		.acpi_match_table = pwm_lpss_acpi_match,
-	},
-	.probe = pwm_lpss_probe_platform,
-	.remove = pwm_lpss_remove_platform,
-};
-
-static int __init pwm_init(void)
-{
-	pci_drv = pci_register_driver(&pwm_lpss_driver_pci);
-	plat_drv = platform_driver_register(&pwm_lpss_driver_platform);
-	if (pci_drv && plat_drv)
-		return pci_drv;
-
-	return 0;
-}
-module_init(pwm_init);
-
-static void __exit pwm_exit(void)
-{
-	if (!pci_drv)
-		pci_unregister_driver(&pwm_lpss_driver_pci);
-	if (!plat_drv)
-		platform_driver_unregister(&pwm_lpss_driver_platform);
-}
-module_exit(pwm_exit);
+EXPORT_SYMBOL_GPL(pwm_lpss_remove);
 
 MODULE_DESCRIPTION("PWM driver for Intel LPSS");
 MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
 MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("platform:pwm-lpss");
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
new file mode 100644
index 0000000..432fe20
--- /dev/null
+++ b/drivers/pwm/pwm-lpss.h
@@ -0,0 +1,32 @@
+/*
+ * Intel Low Power Subsystem PWM controller driver
+ *
+ * Copyright (C) 2014, Intel Corporation
+ *
+ * Derived from the original pwm-lpss.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __PWM_LPSS_H
+#define __PWM_LPSS_H
+
+#include <linux/pwm.h>
+#include <linux/device.h>
+
+struct pwm_lpss_chip;
+
+struct pwm_lpss_boardinfo {
+	unsigned long clk_rate;
+};
+
+extern const struct pwm_lpss_boardinfo pwm_lpss_byt_info;
+extern const struct pwm_lpss_boardinfo pwm_lpss_bsw_info;
+
+extern struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
+		struct resource *r, const struct pwm_lpss_boardinfo *info);
+extern int pwm_lpss_remove(struct pwm_lpss_chip *lpwm);
+
+#endif	/* __PWM_LPSS_H */
-- 
2.1.0

[PATCH v4 2/2] pwm: lpss: pci: move to use pcim_enable_device()

[Andy Shevchenko]

Let's use managed functions for this driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pwm/pwm-lpss-pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
index 12515f3..ac15510 100644
--- a/drivers/pwm/pwm-lpss-pci.c
+++ b/drivers/pwm/pwm-lpss-pci.c
@@ -23,7 +23,7 @@ static int pwm_lpss_probe_pci(struct pci_dev *pdev,
 	struct pwm_lpss_chip *lpwm;
 	int err;
 
-	err = pci_enable_device(pdev);
+	err = pcim_enable_device(pdev);
 	if (err < 0)
 		return err;
 
@@ -41,7 +41,6 @@ static void pwm_lpss_remove_pci(struct pci_dev *pdev)
 	struct pwm_lpss_chip *lpwm = pci_get_drvdata(pdev);
 
 	pwm_lpss_remove(lpwm);
-	pci_disable_device(pdev);
 }
 
 static struct pci_device_id pwm_lpss_pci_ids[] = {
-- 
2.1.0

Re: [PATCH v4 0/2] pwm: lpss: split driver to core and probe drivers

[Thierry Reding]

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

On Tue, Aug 19, 2014 at 07:17:34PM +0300, Andy Shevchenko wrote:
> This small series makes the architecture of pwm-lpss driver cleaner.
> 
> Since v3:
> - rebased on top of recent linux-next
> - rebased on top of Alan's patch for Braswell
> 
> Since v2:
> - fix one more typo in Kconfig (tested with one line removed in my config now)
> 
> Since v1:
> - hide struct definition in the core part
> - rename pwm-lpss-plat to pwm-lpss-platform
> - fix Kconfig dependencies and other typos
> - add Mika's Reviewed-by tag
> - fix spelling in the commit messages
> 
> Andy Shevchenko (2):
>   pwm: lpss: properly split driver to parts
>   pwm: lpss: pci: move to use pcim_enable_device()
> 
>  drivers/pwm/Kconfig             |  21 ++++++-
>  drivers/pwm/Makefile            |   2 +
>  drivers/pwm/pwm-lpss-pci.c      |  65 +++++++++++++++++++
>  drivers/pwm/pwm-lpss-platform.c |  69 ++++++++++++++++++++
>  drivers/pwm/pwm-lpss.c          | 136 +++-------------------------------------
>  drivers/pwm/pwm-lpss.h          |  32 ++++++++++
>  6 files changed, 198 insertions(+), 127 deletions(-)
>  create mode 100644 drivers/pwm/pwm-lpss-pci.c
>  create mode 100644 drivers/pwm/pwm-lpss-platform.c
>  create mode 100644 drivers/pwm/pwm-lpss.h

Yea or nay, Alan?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 0/2] pwm: lpss: split driver to core and probe drivers

[Alan Cox]

On Wed, 2014-08-20 at 12:12 +0200, Thierry Reding wrote:
> On Tue, Aug 19, 2014 at 07:17:34PM +0300, Andy Shevchenko wrote:
> > This small series makes the architecture of pwm-lpss driver cleaner.
> > 
> > Since v3:
> > - rebased on top of recent linux-next
> > - rebased on top of Alan's patch for Braswell
> > 
> > Since v2:
> > - fix one more typo in Kconfig (tested with one line removed in my config now)
> > 
> > Since v1:
> > - hide struct definition in the core part
> > - rename pwm-lpss-plat to pwm-lpss-platform
> > - fix Kconfig dependencies and other typos
> > - add Mika's Reviewed-by tag
> > - fix spelling in the commit messages
> > 
> > Andy Shevchenko (2):
> >   pwm: lpss: properly split driver to parts
> >   pwm: lpss: pci: move to use pcim_enable_device()
> > 
> >  drivers/pwm/Kconfig             |  21 ++++++-
> >  drivers/pwm/Makefile            |   2 +
> >  drivers/pwm/pwm-lpss-pci.c      |  65 +++++++++++++++++++
> >  drivers/pwm/pwm-lpss-platform.c |  69 ++++++++++++++++++++
> >  drivers/pwm/pwm-lpss.c          | 136 +++-------------------------------------
> >  drivers/pwm/pwm-lpss.h          |  32 ++++++++++
> >  6 files changed, 198 insertions(+), 127 deletions(-)
> >  create mode 100644 drivers/pwm/pwm-lpss-pci.c
> >  create mode 100644 drivers/pwm/pwm-lpss-platform.c
> >  create mode 100644 drivers/pwm/pwm-lpss.h
> 
> Yea or nay, Alan?

Context ?

Re: [PATCH v4 0/2] pwm: lpss: split driver to core and probe drivers

[Thierry Reding]

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

On Wed, Aug 20, 2014 at 06:57:59PM +0100, Alan Cox wrote:
> On Wed, 2014-08-20 at 12:12 +0200, Thierry Reding wrote:
> > On Tue, Aug 19, 2014 at 07:17:34PM +0300, Andy Shevchenko wrote:
> > > This small series makes the architecture of pwm-lpss driver cleaner.
> > > 
> > > Since v3:
> > > - rebased on top of recent linux-next
> > > - rebased on top of Alan's patch for Braswell
> > > 
> > > Since v2:
> > > - fix one more typo in Kconfig (tested with one line removed in my config now)
> > > 
> > > Since v1:
> > > - hide struct definition in the core part
> > > - rename pwm-lpss-plat to pwm-lpss-platform
> > > - fix Kconfig dependencies and other typos
> > > - add Mika's Reviewed-by tag
> > > - fix spelling in the commit messages
> > > 
> > > Andy Shevchenko (2):
> > >   pwm: lpss: properly split driver to parts
> > >   pwm: lpss: pci: move to use pcim_enable_device()
> > > 
> > >  drivers/pwm/Kconfig             |  21 ++++++-
> > >  drivers/pwm/Makefile            |   2 +
> > >  drivers/pwm/pwm-lpss-pci.c      |  65 +++++++++++++++++++
> > >  drivers/pwm/pwm-lpss-platform.c |  69 ++++++++++++++++++++
> > >  drivers/pwm/pwm-lpss.c          | 136 +++-------------------------------------
> > >  drivers/pwm/pwm-lpss.h          |  32 ++++++++++
> > >  6 files changed, 198 insertions(+), 127 deletions(-)
> > >  create mode 100644 drivers/pwm/pwm-lpss-pci.c
> > >  create mode 100644 drivers/pwm/pwm-lpss-platform.c
> > >  create mode 100644 drivers/pwm/pwm-lpss.h
> > 
> > Yea or nay, Alan?
> 
> Context ?

Back when the LPSS PWM driver was first submitted I suggested that it
could be split up in this way (core + PCI and ACPI/platform drivers).
You had objections to doing that. Now Andy's doing exactly that, so I
would like your Acked-by (or NAK) on this change.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 0/2] pwm: lpss: split driver to core and probe drivers

[Alan Cox]

On Thu, 2014-08-21 at 08:43 +0200, Thierry Reding wrote:
> On Wed, Aug 20, 2014 at 06:57:59PM +0100, Alan Cox wrote:
> > On Wed, 2014-08-20 at 12:12 +0200, Thierry Reding wrote:
> > > On Tue, Aug 19, 2014 at 07:17:34PM +0300, Andy Shevchenko wrote:
> > > > This small series makes the architecture of pwm-lpss driver cleaner.
> > > > 
> > > > Since v3:
> > > > - rebased on top of recent linux-next
> > > > - rebased on top of Alan's patch for Braswell
> > > > 
> > > > Since v2:
> > > > - fix one more typo in Kconfig (tested with one line removed in my config now)
> > > > 
> > > > Since v1:
> > > > - hide struct definition in the core part
> > > > - rename pwm-lpss-plat to pwm-lpss-platform
> > > > - fix Kconfig dependencies and other typos
> > > > - add Mika's Reviewed-by tag
> > > > - fix spelling in the commit messages
> > > > 
> > > > Andy Shevchenko (2):
> > > >   pwm: lpss: properly split driver to parts
> > > >   pwm: lpss: pci: move to use pcim_enable_device()
> > > > 
> > > >  drivers/pwm/Kconfig             |  21 ++++++-
> > > >  drivers/pwm/Makefile            |   2 +
> > > >  drivers/pwm/pwm-lpss-pci.c      |  65 +++++++++++++++++++
> > > >  drivers/pwm/pwm-lpss-platform.c |  69 ++++++++++++++++++++
> > > >  drivers/pwm/pwm-lpss.c          | 136 +++-------------------------------------
> > > >  drivers/pwm/pwm-lpss.h          |  32 ++++++++++
> > > >  6 files changed, 198 insertions(+), 127 deletions(-)
> > > >  create mode 100644 drivers/pwm/pwm-lpss-pci.c
> > > >  create mode 100644 drivers/pwm/pwm-lpss-platform.c
> > > >  create mode 100644 drivers/pwm/pwm-lpss.h
> > > 
> > > Yea or nay, Alan?
> > 
> > Context ?
> 
> Back when the LPSS PWM driver was first submitted I suggested that it
> could be split up in this way (core + PCI and ACPI/platform drivers).
> You had objections to doing that. Now Andy's doing exactly that, so I
> would like your Acked-by (or NAK) on this change.

I think Andy proved you were right 8)

Ack from me

Alan

Re: [PATCH v4 1/2] pwm: lpss: properly split driver to parts

[Thierry Reding]

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

On Tue, Aug 19, 2014 at 07:17:35PM +0300, Andy Shevchenko wrote:
[...]
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
[...]
> +config PWM_LPSS_PCI
> +	tristate "Intel LPSS PWM PCI driver"
> +	depends on PCI
> +	select PWM_LPSS
[...]
> +config PWM_LPSS_PLATFORM
> +	tristate "Intel LPSS PWM platform driver"
> +	depends on ACPI
> +	select PWM_LPSS

I changed both of the above select PWM_LPSS to depends on PWM_LPSS
because that makes them show up in a more meaningful way in menuconfig
and otherwise it looks weird if the PWM_LPSS shows up as automatically
selected without being useful in itself. This way the PWM_LPSS is sort
of a common core that PWM_LPSS_PLATFORM and PWM_LPSS_PCI use.

An alternative that's commonly used for this would be to hide PWM_LPSS
from users and keep the select within PWM_LPSS_PLATFORM and
PWM_LPSS_PCI. That would be okay with me too.

Let me know if you have any objections to this change.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 1/2] pwm: lpss: properly split driver to parts

[Andy Shevchenko]

On Fri, 2014-08-22 at 13:13 +0200, Thierry Reding wrote:
> On Tue, Aug 19, 2014 at 07:17:35PM +0300, Andy Shevchenko wrote:
> [...]
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> [...]
> > +config PWM_LPSS_PCI
> > +	tristate "Intel LPSS PWM PCI driver"
> > +	depends on PCI
> > +	select PWM_LPSS
> [...]
> > +config PWM_LPSS_PLATFORM
> > +	tristate "Intel LPSS PWM platform driver"
> > +	depends on ACPI
> > +	select PWM_LPSS
> 
> I changed both of the above select PWM_LPSS to depends on PWM_LPSS
> because that makes them show up in a more meaningful way in menuconfig
> and otherwise it looks weird if the PWM_LPSS shows up as automatically
> selected without being useful in itself. This way the PWM_LPSS is sort
> of a common core that PWM_LPSS_PLATFORM and PWM_LPSS_PCI use.
> 
> An alternative that's commonly used for this would be to hide PWM_LPSS
> from users and keep the select within PWM_LPSS_PLATFORM and
> PWM_LPSS_PCI. That would be okay with me too.
> 
> Let me know if you have any objections to this change.


Didn't notice your message before.
Regarding to this one I think the way to hide PWM_LPSS from user would
be preferred, though I'm okay if it keeps visible.

In addition to compile error you get from Stephen, thanks for quick fix,
though I think we have to make PWM_LPSS dependent to X86 for now. I
don't believe we will have same IP on other architectures.


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

Re: [PATCH v4 1/2] pwm: lpss: properly split driver to parts

[Thierry Reding]

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

On Mon, Aug 25, 2014 at 11:11:17AM +0300, Andy Shevchenko wrote:
> On Fri, 2014-08-22 at 13:13 +0200, Thierry Reding wrote:
> > On Tue, Aug 19, 2014 at 07:17:35PM +0300, Andy Shevchenko wrote:
> > [...]
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > [...]
> > > +config PWM_LPSS_PCI
> > > +	tristate "Intel LPSS PWM PCI driver"
> > > +	depends on PCI
> > > +	select PWM_LPSS
> > [...]
> > > +config PWM_LPSS_PLATFORM
> > > +	tristate "Intel LPSS PWM platform driver"
> > > +	depends on ACPI
> > > +	select PWM_LPSS
> > 
> > I changed both of the above select PWM_LPSS to depends on PWM_LPSS
> > because that makes them show up in a more meaningful way in menuconfig
> > and otherwise it looks weird if the PWM_LPSS shows up as automatically
> > selected without being useful in itself. This way the PWM_LPSS is sort
> > of a common core that PWM_LPSS_PLATFORM and PWM_LPSS_PCI use.
> > 
> > An alternative that's commonly used for this would be to hide PWM_LPSS
> > from users and keep the select within PWM_LPSS_PLATFORM and
> > PWM_LPSS_PCI. That would be okay with me too.
> > 
> > Let me know if you have any objections to this change.
> 
> 
> Didn't notice your message before.
> Regarding to this one I think the way to hide PWM_LPSS from user would
> be preferred, though I'm okay if it keeps visible.
> 
> In addition to compile error you get from Stephen, thanks for quick fix,
> though I think we have to make PWM_LPSS dependent to X86 for now. I
> don't believe we will have same IP on other architectures.

I does seem to build fine for other architectures, so I'm fine with
keeping it available for non-x86. But I don't have a strong opinion
either way, so if you'd rather make it x86-specific, feel free to
send a patch.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]