[PATCH] pwm_lpss: Add support for PCI devices

[PATCH] pwm_lpss: Add support for PCI devices

[Chew Chiau Ee]

From: Alan Cox <alan@linux.intel.com>

Not all systems enumerate the PWM devices via ACPI. They can also be exposed
via the PCI interface.

Signed-off-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
---
 drivers/pwm/pwm-lpss.c |  160 ++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 129 insertions(+), 31 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 449e372..6f79bf8 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -6,6 +6,7 @@
  * Author: Chew Kean Ho <kean.ho.chew@intel.com>
  * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
  * Author: Chew Chiau Ee <chiau.ee.chew@intel.com>
+ * Author: Alan Cox <alan@linux.intel.com>
  *
  * 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
@@ -19,6 +20,9 @@
 #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 */
 
 #define PWM				0x00000000
 #define PWM_ENABLE			BIT(31)
@@ -34,6 +38,15 @@ struct pwm_lpss_chip {
 	struct pwm_chip chip;
 	void __iomem *regs;
 	struct clk *clk;
+	unsigned long clk_rate;
+};
+
+struct pwm_lpss_boardinfo {
+	unsigned long clk_rate;
+};
+
+static const struct pwm_lpss_boardinfo byt_info = {
+	25000000
 };
 
 static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
@@ -55,7 +68,7 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	/* The equation is: base_unit = ((freq / c) * 65536) + correction */
 	base_unit = freq * 65536;
 
-	c = clk_get_rate(lpwm->clk);
+	c = lpwm->clk_rate;
 	if (!c)
 		return -EINVAL;
 
@@ -113,52 +126,47 @@ static const struct pwm_ops pwm_lpss_ops = {
 	.owner = THIS_MODULE,
 };
 
-static const struct acpi_device_id pwm_lpss_acpi_match[] = {
-	{ "80860F09", 0 },
-	{ },
-};
-MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match);
-
-static int pwm_lpss_probe(struct platform_device *pdev)
+static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
+			struct resource *r, struct pwm_lpss_boardinfo *info)
 {
 	struct pwm_lpss_chip *lpwm;
-	struct resource *r;
 	int ret;
 
-	lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL);
+	lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL);
 	if (!lpwm)
-		return -ENOMEM;
-
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		return ERR_PTR(-ENOMEM);
 
-	lpwm->regs = devm_ioremap_resource(&pdev->dev, r);
+	lpwm->regs = devm_ioremap_resource(dev, r);
 	if (IS_ERR(lpwm->regs))
-		return PTR_ERR(lpwm->regs);
-
-	lpwm->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(lpwm->clk)) {
-		dev_err(&pdev->dev, "failed to get PWM clock\n");
-		return PTR_ERR(lpwm->clk);
+		return lpwm->regs;
+
+	if (info) {
+		lpwm->clk_rate = info->clk_rate;
+	} else {
+		lpwm->clk = devm_clk_get(dev, NULL);
+		if (IS_ERR(lpwm->clk)) {
+			dev_err(dev, "failed to get PWM clock\n");
+			return (void *)lpwm->clk;
+		}
+		lpwm->clk_rate = clk_get_rate(lpwm->clk);
 	}
 
-	lpwm->chip.dev = &pdev->dev;
+	lpwm->chip.dev = dev;
 	lpwm->chip.ops = &pwm_lpss_ops;
 	lpwm->chip.base = -1;
 	lpwm->chip.npwm = 1;
 
 	ret = pwmchip_add(&lpwm->chip);
 	if (ret) {
-		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
-		return ret;
+		dev_err(dev, "failed to add PWM chip: %d\n", ret);
+		return ERR_PTR(ret);
 	}
 
-	platform_set_drvdata(pdev, lpwm);
-	return 0;
+	return lpwm;
 }
 
-static int pwm_lpss_remove(struct platform_device *pdev)
+static int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
 {
-	struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev);
 	u32 ctrl;
 
 	ctrl = readl(lpwm->regs + PWM);
@@ -167,17 +175,107 @@ static int pwm_lpss_remove(struct platform_device *pdev)
 	return pwmchip_remove(&lpwm->chip);
 }
 
-static struct platform_driver pwm_lpss_driver = {
+static int pwm_lpss_probe_pci(struct pci_dev *pdev,
+			      const struct pci_device_id *id)
+{
+	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},
+	{ 0,}
+};
+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)
+{
+	struct pwm_lpss_chip *lpwm;
+	struct resource *r;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	lpwm = pwm_lpss_probe(&pdev->dev, r, NULL);
+	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", 0 },
+	{ },
+};
+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,
-	.remove = pwm_lpss_remove,
+	.probe = pwm_lpss_probe_platform,
+	.remove = pwm_lpss_remove_platform,
 };
-module_platform_driver(pwm_lpss_driver);
+
+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;
+}
+
+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_DESCRIPTION("PWM driver for Intel LPSS");
 MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
 MODULE_LICENSE("GPL v2");
 MODULE_ALIAS("platform:pwm-lpss");
+
+module_init(pwm_init);
+module_exit(pwm_exit);
-- 
1.7.4.4

Re: [PATCH] pwm_lpss: Add support for PCI devices

[Li, Aubrey]

On 2014/4/12 21:58, Chew Chiau Ee wrote:
> From: Alan Cox <alan@linux.intel.com>
> 
> Not all systems enumerate the PWM devices via ACPI. They can also be exposed
> via the PCI interface.
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> ---
>  drivers/pwm/pwm-lpss.c |  160 ++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 129 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 449e372..6f79bf8 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -6,6 +6,7 @@
>   * Author: Chew Kean Ho <kean.ho.chew@intel.com>
>   * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
>   * Author: Chew Chiau Ee <chiau.ee.chew@intel.com>
> + * Author: Alan Cox <alan@linux.intel.com>
>   *
>   * 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
> @@ -19,6 +20,9 @@
>  #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 */
>  
>  #define PWM				0x00000000
>  #define PWM_ENABLE			BIT(31)
> @@ -34,6 +38,15 @@ struct pwm_lpss_chip {
>  	struct pwm_chip chip;
>  	void __iomem *regs;
>  	struct clk *clk;
> +	unsigned long clk_rate;
> +};
> +
> +struct pwm_lpss_boardinfo {
> +	unsigned long clk_rate;
> +};
> +
> +static const struct pwm_lpss_boardinfo byt_info = {
> +	25000000
>  };
>  
>  static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
> @@ -55,7 +68,7 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	/* The equation is: base_unit = ((freq / c) * 65536) + correction */
>  	base_unit = freq * 65536;
>  
> -	c = clk_get_rate(lpwm->clk);
> +	c = lpwm->clk_rate;
>  	if (!c)
>  		return -EINVAL;
>  
> @@ -113,52 +126,47 @@ static const struct pwm_ops pwm_lpss_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> -static const struct acpi_device_id pwm_lpss_acpi_match[] = {
> -	{ "80860F09", 0 },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match);
> -
> -static int pwm_lpss_probe(struct platform_device *pdev)
> +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
> +			struct resource *r, struct pwm_lpss_boardinfo *info)
>  {
>  	struct pwm_lpss_chip *lpwm;
> -	struct resource *r;
>  	int ret;
>  
> -	lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL);
> +	lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL);
>  	if (!lpwm)
> -		return -ENOMEM;
> -
> -	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		return ERR_PTR(-ENOMEM);
>  
> -	lpwm->regs = devm_ioremap_resource(&pdev->dev, r);
> +	lpwm->regs = devm_ioremap_resource(dev, r);
>  	if (IS_ERR(lpwm->regs))
> -		return PTR_ERR(lpwm->regs);
> -
> -	lpwm->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(lpwm->clk)) {
> -		dev_err(&pdev->dev, "failed to get PWM clock\n");
> -		return PTR_ERR(lpwm->clk);
> +		return lpwm->regs;
> +
> +	if (info) {
> +		lpwm->clk_rate = info->clk_rate;
> +	} else {
> +		lpwm->clk = devm_clk_get(dev, NULL);
> +		if (IS_ERR(lpwm->clk)) {
> +			dev_err(dev, "failed to get PWM clock\n");
> +			return (void *)lpwm->clk;
> +		}
> +		lpwm->clk_rate = clk_get_rate(lpwm->clk);
>  	}
>  
> -	lpwm->chip.dev = &pdev->dev;
> +	lpwm->chip.dev = dev;
>  	lpwm->chip.ops = &pwm_lpss_ops;
>  	lpwm->chip.base = -1;
>  	lpwm->chip.npwm = 1;
>  
>  	ret = pwmchip_add(&lpwm->chip);
>  	if (ret) {
> -		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
> -		return ret;
> +		dev_err(dev, "failed to add PWM chip: %d\n", ret);
> +		return ERR_PTR(ret);
>  	}
>  
> -	platform_set_drvdata(pdev, lpwm);
> -	return 0;
> +	return lpwm;
>  }
>  
> -static int pwm_lpss_remove(struct platform_device *pdev)
> +static int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
>  {
> -	struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev);
>  	u32 ctrl;
>  
>  	ctrl = readl(lpwm->regs + PWM);
> @@ -167,17 +175,107 @@ static int pwm_lpss_remove(struct platform_device *pdev)
>  	return pwmchip_remove(&lpwm->chip);
>  }
>  
> -static struct platform_driver pwm_lpss_driver = {
> +static int pwm_lpss_probe_pci(struct pci_dev *pdev,
> +			      const struct pci_device_id *id)
> +{
> +	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},
> +	{ 0,}
> +};
> +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)
> +{
> +	struct pwm_lpss_chip *lpwm;
> +	struct resource *r;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	lpwm = pwm_lpss_probe(&pdev->dev, r, NULL);
> +	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", 0 },
> +	{ },
> +};
> +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,
> -	.remove = pwm_lpss_remove,
> +	.probe = pwm_lpss_probe_platform,
> +	.remove = pwm_lpss_remove_platform,
>  };
> -module_platform_driver(pwm_lpss_driver);
> +
> +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;
> +}
> +
> +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_DESCRIPTION("PWM driver for Intel LPSS");
>  MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
>  MODULE_LICENSE("GPL v2");
>  MODULE_ALIAS("platform:pwm-lpss");

Looks a good idea to combine pci and acpi driver together.
Since pci driver is added, here the alias need to be refined.
Others look good.

Thanks,
-Aubrey

> +
> +module_init(pwm_init);
> +module_exit(pwm_exit);
> 

RE: [PATCH] pwm_lpss: Add support for PCI devices

[Chew, Chiau Ee]

> >  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");
> 
> Looks a good idea to combine pci and acpi driver together.
> Since pci driver is added, here the alias need to be refined.
> Others look good.
> 
> Thanks,
> -Aubrey

Ok. I will change it to MODULE_ALIAS("pci/platform:pwm-lpss");

Thanks,
Chiau Ee

Re: [PATCH] pwm_lpss: Add support for PCI devices

[Mika Westerberg]

On Sat, Apr 12, 2014 at 09:58:51PM +0800, Chew Chiau Ee wrote:
> From: Alan Cox <alan@linux.intel.com>
> 
> Not all systems enumerate the PWM devices via ACPI. They can also be exposed
> via the PCI interface.
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> ---
>  drivers/pwm/pwm-lpss.c |  160 ++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 129 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 449e372..6f79bf8 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -6,6 +6,7 @@
>   * Author: Chew Kean Ho <kean.ho.chew@intel.com>
>   * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
>   * Author: Chew Chiau Ee <chiau.ee.chew@intel.com>
> + * Author: Alan Cox <alan@linux.intel.com>
>   *
>   * 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
> @@ -19,6 +20,9 @@
>  #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 */
>  
>  #define PWM				0x00000000
>  #define PWM_ENABLE			BIT(31)
> @@ -34,6 +38,15 @@ struct pwm_lpss_chip {
>  	struct pwm_chip chip;
>  	void __iomem *regs;
>  	struct clk *clk;
> +	unsigned long clk_rate;
> +};
> +
> +struct pwm_lpss_boardinfo {
> +	unsigned long clk_rate;
> +};
> +
> +static const struct pwm_lpss_boardinfo byt_info = {
> +	25000000
>  };
>  
>  static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
> @@ -55,7 +68,7 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	/* The equation is: base_unit = ((freq / c) * 65536) + correction */
>  	base_unit = freq * 65536;
>  
> -	c = clk_get_rate(lpwm->clk);
> +	c = lpwm->clk_rate;
>  	if (!c)
>  		return -EINVAL;
>  
> @@ -113,52 +126,47 @@ static const struct pwm_ops pwm_lpss_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> -static const struct acpi_device_id pwm_lpss_acpi_match[] = {
> -	{ "80860F09", 0 },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match);
> -
> -static int pwm_lpss_probe(struct platform_device *pdev)
> +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
> +			struct resource *r, struct pwm_lpss_boardinfo *info)
>  {
>  	struct pwm_lpss_chip *lpwm;
> -	struct resource *r;
>  	int ret;
>  
> -	lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL);
> +	lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL);
>  	if (!lpwm)
> -		return -ENOMEM;
> -
> -	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		return ERR_PTR(-ENOMEM);
>  
> -	lpwm->regs = devm_ioremap_resource(&pdev->dev, r);
> +	lpwm->regs = devm_ioremap_resource(dev, r);
>  	if (IS_ERR(lpwm->regs))
> -		return PTR_ERR(lpwm->regs);
> -
> -	lpwm->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(lpwm->clk)) {
> -		dev_err(&pdev->dev, "failed to get PWM clock\n");
> -		return PTR_ERR(lpwm->clk);
> +		return lpwm->regs;
> +
> +	if (info) {
> +		lpwm->clk_rate = info->clk_rate;
> +	} else {
> +		lpwm->clk = devm_clk_get(dev, NULL);
> +		if (IS_ERR(lpwm->clk)) {
> +			dev_err(dev, "failed to get PWM clock\n");
> +			return (void *)lpwm->clk;

Why the cast here?

> +		}
> +		lpwm->clk_rate = clk_get_rate(lpwm->clk);
>  	}
>  
> -	lpwm->chip.dev = &pdev->dev;
> +	lpwm->chip.dev = dev;
>  	lpwm->chip.ops = &pwm_lpss_ops;
>  	lpwm->chip.base = -1;
>  	lpwm->chip.npwm = 1;
>  
>  	ret = pwmchip_add(&lpwm->chip);
>  	if (ret) {
> -		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
> -		return ret;
> +		dev_err(dev, "failed to add PWM chip: %d\n", ret);
> +		return ERR_PTR(ret);
>  	}
>  
> -	platform_set_drvdata(pdev, lpwm);
> -	return 0;
> +	return lpwm;
>  }
>  
> -static int pwm_lpss_remove(struct platform_device *pdev)
> +static int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
>  {
> -	struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev);
>  	u32 ctrl;
>  
>  	ctrl = readl(lpwm->regs + PWM);
> @@ -167,17 +175,107 @@ static int pwm_lpss_remove(struct platform_device *pdev)
>  	return pwmchip_remove(&lpwm->chip);
>  }
>  
> -static struct platform_driver pwm_lpss_driver = {
> +static int pwm_lpss_probe_pci(struct pci_dev *pdev,
> +			      const struct pci_device_id *id)
> +{
> +	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},

I think non-capital letters for hex numbers are more consistent. I.e
0x0f08.

> +	{ 0,}

{ },

as in the platform part looks better, IMO.

> +};
> +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,

The platform part doesn't use tabs, so I think you should follow the style
here.

> +	.probe	= pwm_lpss_probe_pci,
> +	.remove	= pwm_lpss_remove_pci,
> +};
> +
> +static int pwm_lpss_probe_platform(struct platform_device *pdev)
> +{
> +	struct pwm_lpss_chip *lpwm;
> +	struct resource *r;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	lpwm = pwm_lpss_probe(&pdev->dev, r, NULL);
> +	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", 0 },
> +	{ },
> +};
> +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,
> -	.remove = pwm_lpss_remove,
> +	.probe = pwm_lpss_probe_platform,
> +	.remove = pwm_lpss_remove_platform,
>  };
> -module_platform_driver(pwm_lpss_driver);
> +
> +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;
> +}
> +
> +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_DESCRIPTION("PWM driver for Intel LPSS");
>  MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
>  MODULE_LICENSE("GPL v2");
>  MODULE_ALIAS("platform:pwm-lpss");
> +
> +module_init(pwm_init);
> +module_exit(pwm_exit);

Maybe move these to follow the function defitions, like:

pwm_init()
{
}
module_init(pwm_init);

and so on.

Re: [PATCH] pwm_lpss: Add support for PCI devices

[Mika Westerberg]

On Mon, Apr 14, 2014 at 02:05:25AM +0000, Chew, Chiau Ee wrote:
> > >  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");
> > 
> > Looks a good idea to combine pci and acpi driver together.
> > Since pci driver is added, here the alias need to be refined.
> > Others look good.
> > 
> > Thanks,
> > -Aubrey
> 
> Ok. I will change it to MODULE_ALIAS("pci/platform:pwm-lpss");

Hmm, does that really work like that?

For PCI and ACPI you already have tables with MODULE_DEVICE_TABLE().

For pure platform driver (which is probably not going to be used) you nede
to have MODULE_ALIAS() if you want modprobe to load it automagically.

RE: [PATCH] pwm_lpss: Add support for PCI devices

[Chew, Chiau Ee]

> > +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
> > +			struct resource *r, struct pwm_lpss_boardinfo *info)
> >  {
> >  	struct pwm_lpss_chip *lpwm;
> > -	struct resource *r;
> >  	int ret;
> >
> > -	lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL);
> > +	lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL);
> >  	if (!lpwm)
> > -		return -ENOMEM;
> > -
> > -	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +		return ERR_PTR(-ENOMEM);
> >
> > -	lpwm->regs = devm_ioremap_resource(&pdev->dev, r);
> > +	lpwm->regs = devm_ioremap_resource(dev, r);
> >  	if (IS_ERR(lpwm->regs))
> > -		return PTR_ERR(lpwm->regs);
> > -
> > -	lpwm->clk = devm_clk_get(&pdev->dev, NULL);
> > -	if (IS_ERR(lpwm->clk)) {
> > -		dev_err(&pdev->dev, "failed to get PWM clock\n");
> > -		return PTR_ERR(lpwm->clk);
> > +		return lpwm->regs;
> > +
> > +	if (info) {
> > +		lpwm->clk_rate = info->clk_rate;
> > +	} else {
> > +		lpwm->clk = devm_clk_get(dev, NULL);
> > +		if (IS_ERR(lpwm->clk)) {
> > +			dev_err(dev, "failed to get PWM clock\n");
> > +			return (void *)lpwm->clk;
> 
> Why the cast here?

Alan, please do correct me if I describe wrongly on the purpose that you add the cast here.

As you can see, pwm_lpss_probe() is expected to return a pointer of type struct pwm_lpss_chip.  On the other hand,   
the return of devm_clk_get() would be a pointer of type struct clk. So if were to use lpwm->clk as the return value of pwm_lpss_probe() , 
a cast would be needed since the pointer type is different from the expected one. The compiler would complain on " warning: return from incompatible pointer type"
if without the cast.

> > +
> > +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},
> 
> I think non-capital letters for hex numbers are more consistent. I.e 0x0f08.
> 

Ok. Noted.

> > +	{ 0,}
> 
> { },
> 
> as in the platform part looks better, IMO.

Ok. Noted.

> 
> > +};
> > +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,
> 
> The platform part doesn't use tabs, so I think you should follow the style here.
> 

Ok. Noted

> >
> >  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");
> > +
> > +module_init(pwm_init);
> > +module_exit(pwm_exit);
> 
> Maybe move these to follow the function defitions, like:
> 
> pwm_init()
> {
> }
> module_init(pwm_init);
> 
> and so on.

Ok. Noted.

RE: [PATCH] pwm_lpss: Add support for PCI devices

[Chew, Chiau Ee]

> On Mon, Apr 14, 2014 at 02:05:25AM +0000, Chew, Chiau Ee wrote:
> > > >  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");
> > >
> > > Looks a good idea to combine pci and acpi driver together.
> > > Since pci driver is added, here the alias need to be refined.
> > > Others look good.
> > >
> > > Thanks,
> > > -Aubrey
> >
> > Ok. I will change it to MODULE_ALIAS("pci/platform:pwm-lpss");
> 
> Hmm, does that really work like that?
> 
> For PCI and ACPI you already have tables with MODULE_DEVICE_TABLE().
> 
> For pure platform driver (which is probably not going to be used) you nede to
> have MODULE_ALIAS() if you want modprobe to load it automagically.

Ok, looks like MODULE_ALIAS() is a redundant. I will remove it.

Re: [PATCH] pwm_lpss: Add support for PCI devices

[Mika Westerberg]

On Tue, Apr 15, 2014 at 08:41:02AM +0000, Chew, Chiau Ee wrote:
> 
> 
> > > +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
> > > +			struct resource *r, struct pwm_lpss_boardinfo *info)
> > >  {
> > >  	struct pwm_lpss_chip *lpwm;
> > > -	struct resource *r;
> > >  	int ret;
> > >
> > > -	lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL);
> > > +	lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL);
> > >  	if (!lpwm)
> > > -		return -ENOMEM;
> > > -
> > > -	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +		return ERR_PTR(-ENOMEM);
> > >
> > > -	lpwm->regs = devm_ioremap_resource(&pdev->dev, r);
> > > +	lpwm->regs = devm_ioremap_resource(dev, r);
> > >  	if (IS_ERR(lpwm->regs))
> > > -		return PTR_ERR(lpwm->regs);
> > > -
> > > -	lpwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > -	if (IS_ERR(lpwm->clk)) {
> > > -		dev_err(&pdev->dev, "failed to get PWM clock\n");
> > > -		return PTR_ERR(lpwm->clk);
> > > +		return lpwm->regs;
> > > +
> > > +	if (info) {
> > > +		lpwm->clk_rate = info->clk_rate;
> > > +	} else {
> > > +		lpwm->clk = devm_clk_get(dev, NULL);
> > > +		if (IS_ERR(lpwm->clk)) {
> > > +			dev_err(dev, "failed to get PWM clock\n");
> > > +			return (void *)lpwm->clk;
> > 
> > Why the cast here?
> 
> Alan, please do correct me if I describe wrongly on the purpose that you add the cast here.
> 
> As you can see, pwm_lpss_probe() is expected to return a pointer of type struct pwm_lpss_chip.  On the other hand,   
> the return of devm_clk_get() would be a pointer of type struct clk. So if were to use lpwm->clk as the return value of pwm_lpss_probe() , 
> a cast would be needed since the pointer type is different from the expected one. The compiler would complain on " warning: return from incompatible pointer type"
> if without the cast.

I think you can use ERR_CAST() here.

RE: [PATCH] pwm_lpss: Add support for PCI devices

[Chew, Chiau Ee]

> On Tue, Apr 15, 2014 at 08:41:02AM +0000, Chew, Chiau Ee wrote:
> >
> >
> > > > +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
> > > > +			struct resource *r, struct pwm_lpss_boardinfo *info)
> > > >  {
> > > >  	struct pwm_lpss_chip *lpwm;
> > > > -	struct resource *r;
> > > >  	int ret;
> > > >
> > > > -	lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL);
> > > > +	lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL);
> > > >  	if (!lpwm)
> > > > -		return -ENOMEM;
> > > > -
> > > > -	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > +		return ERR_PTR(-ENOMEM);
> > > >
> > > > -	lpwm->regs = devm_ioremap_resource(&pdev->dev, r);
> > > > +	lpwm->regs = devm_ioremap_resource(dev, r);
> > > >  	if (IS_ERR(lpwm->regs))
> > > > -		return PTR_ERR(lpwm->regs);
> > > > -
> > > > -	lpwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > > -	if (IS_ERR(lpwm->clk)) {
> > > > -		dev_err(&pdev->dev, "failed to get PWM clock\n");
> > > > -		return PTR_ERR(lpwm->clk);
> > > > +		return lpwm->regs;
> > > > +
> > > > +	if (info) {
> > > > +		lpwm->clk_rate = info->clk_rate;
> > > > +	} else {
> > > > +		lpwm->clk = devm_clk_get(dev, NULL);
> > > > +		if (IS_ERR(lpwm->clk)) {
> > > > +			dev_err(dev, "failed to get PWM clock\n");
> > > > +			return (void *)lpwm->clk;
> > >
> > > Why the cast here?
> >
> > Alan, please do correct me if I describe wrongly on the purpose that you add
> the cast here.
> >
> > As you can see, pwm_lpss_probe() is expected to return a pointer of type
> struct pwm_lpss_chip.  On the other hand,
> > the return of devm_clk_get() would be a pointer of type struct clk. So
> > if were to use lpwm->clk as the return value of pwm_lpss_probe() , a cast
> would be needed since the pointer type is different from the expected one. The
> compiler would complain on " warning: return from incompatible pointer type"
> > if without the cast.
> 
> I think you can use ERR_CAST() here.

You are right....ERR_CAST() can be used. 

Ok...i will clean up the patch and resend a v2 version.

Thanks for the review and feedback.

Re: [PATCH] pwm_lpss: Add support for PCI devices

[Darren Hart]

On Sat, Apr 12, 2014 at 09:58:51PM +0800, Chew Chiau Ee wrote:
> From: Alan Cox <alan@linux.intel.com>
> 
> Not all systems enumerate the PWM devices via ACPI. They can also be exposed
> via the PCI interface.
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> ---
>  drivers/pwm/pwm-lpss.c |  160 ++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 129 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 449e372..6f79bf8 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -6,6 +6,7 @@
>   * Author: Chew Kean Ho <kean.ho.chew@intel.com>
>   * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
>   * Author: Chew Chiau Ee <chiau.ee.chew@intel.com>
> + * Author: Alan Cox <alan@linux.intel.com>
>   *
>   * 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
> @@ -19,6 +20,9 @@
>  #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 */
>  
>  #define PWM				0x00000000
>  #define PWM_ENABLE			BIT(31)
> @@ -34,6 +38,15 @@ struct pwm_lpss_chip {
>  	struct pwm_chip chip;
>  	void __iomem *regs;
>  	struct clk *clk;
> +	unsigned long clk_rate;
> +};
> +
> +struct pwm_lpss_boardinfo {
> +	unsigned long clk_rate;
> +};
> +
> +static const struct pwm_lpss_boardinfo byt_info = {
> +	25000000
>  };
>  
>  static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
> @@ -55,7 +68,7 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	/* The equation is: base_unit = ((freq / c) * 65536) + correction */
>  	base_unit = freq * 65536;
>  
> -	c = clk_get_rate(lpwm->clk);
> +	c = lpwm->clk_rate;
>  	if (!c)
>  		return -EINVAL;
>  
> @@ -113,52 +126,47 @@ static const struct pwm_ops pwm_lpss_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> -static const struct acpi_device_id pwm_lpss_acpi_match[] = {
> -	{ "80860F09", 0 },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match);
> -
> -static int pwm_lpss_probe(struct platform_device *pdev)
> +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
> +			struct resource *r, struct pwm_lpss_boardinfo *info)

Has this landed anywhere? I didn't see it in mainline or next, am I looking in
the wrong place? If it's just still pending, I ran into one issue integrating it
with 3.14.2:

  CC [M]  drivers/pwm/pwm-lpss.o
  drivers/pwm/pwm-lpss.c: In function ‘pwm_lpss_probe_pci’:
  drivers/pwm/pwm-lpss.c:192:2: warning: passing argument 3 of ‘pwm_lpss_probe’
  discards ‘const’ qualifier from pointer target type [enabled by default]
  drivers/pwm/pwm-lpss.c:130:30: note: expected ‘struct pwm_lpss_boardinfo *’
  but argument is of type ‘const struct pwm_lpss_boardinfo *’

Can we make the third argument to pwm_lpss_probe a const? The following is
working for me:

 static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
                                             struct resource *r,
                                             const struct pwm_lpss_boardinfo *info)

Thanks,

--
Darren Hart			Open Source Technology Center
darren.hart@intel.com		Intel Corporation

Re: [PATCH] pwm_lpss: Add support for PCI devices

[Thierry Reding]

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

On Mon, May 12, 2014 at 04:18:16PM -0700, Darren Hart wrote:
> On Sat, Apr 12, 2014 at 09:58:51PM +0800, Chew Chiau Ee wrote:
> > From: Alan Cox <alan@linux.intel.com>
> > 
> > Not all systems enumerate the PWM devices via ACPI. They can also be exposed
> > via the PCI interface.
> > 
> > Signed-off-by: Alan Cox <alan@linux.intel.com>
> > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > ---
> >  drivers/pwm/pwm-lpss.c |  160 ++++++++++++++++++++++++++++++++++++++---------
> >  1 files changed, 129 insertions(+), 31 deletions(-)
[...]
> Has this landed anywhere? I didn't see it in mainline or next, am I looking in
> the wrong place?

This was applied to the pwm/for-next tree and has been in linux-next
since next-20140429.

> If it's just still pending, I ran into one issue integrating it
> with 3.14.2:
> 
>   CC [M]  drivers/pwm/pwm-lpss.o
>   drivers/pwm/pwm-lpss.c: In function ‘pwm_lpss_probe_pci’:
>   drivers/pwm/pwm-lpss.c:192:2: warning: passing argument 3 of ‘pwm_lpss_probe’
>   discards ‘const’ qualifier from pointer target type [enabled by default]
>   drivers/pwm/pwm-lpss.c:130:30: note: expected ‘struct pwm_lpss_boardinfo *’
>   but argument is of type ‘const struct pwm_lpss_boardinfo *’
> 
> Can we make the third argument to pwm_lpss_probe a const? The following is
> working for me:
> 
>  static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
>                                              struct resource *r,
>                                              const struct pwm_lpss_boardinfo *info)

A fix like that was applied last week and is in next-20140512.

Thierry

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