[PATCH V3 0/3] pwm: pxa: bug fix and device tree support

[PATCH V3 0/3] pwm: pxa: bug fix and device tree support

[Chao Xie]

The patches fix some bugs
1. pwm-pxa driver is shared by ARCH_PXA and ARCH_MMP
2. use module_platform_driver for driver register

The patches also add device tree support for pwm.

V2->V1:
  remove the redundant initialization.
  fix bug of no initialization of .of_match_table
  Use CONFIG_OF for device tree support. 
  rebase to for-next

V3->V2:
  change "mrvl,xxx" to "marvell,xxx".

Chao Xie (3):
  pwm: pxa: ARCH_MMP share same pwm driver with ARCH_PXA
  pwm: pxa: use module_platform_driver for driver register
  pwm: pxa: add device tree support

 drivers/pwm/Kconfig   |    2 +-
 drivers/pwm/pwm-pxa.c |   69 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 57 insertions(+), 14 deletions(-)

-- 
1.7.4.1

[PATCH V3 1/3] pwm: pxa: ARCH_MMP share same pwm driver with ARCH_PXA

[Chao Xie]

the pwm driver is not only used by ARCH_PXA but also ARCH_MMP

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 drivers/pwm/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 115b644..9ec4040 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -108,7 +108,7 @@ config PWM_PUV3
 
 config PWM_PXA
 	tristate "PXA PWM support"
-	depends on ARCH_PXA
+	depends on ARCH_PXA || ARCH_MMP
 	help
 	  Generic PWM framework driver for PXA.
 
-- 
1.7.4.1

Re: [PATCH V3 1/3] pwm: pxa: ARCH_MMP share same pwm driver with ARCH_PXA

[Thierry Reding]

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

On Tue, Apr 23, 2013 at 10:23:52PM -0400, Chao Xie wrote:
> the pwm driver is not only used by ARCH_PXA but also ARCH_MMP

Please use only uppercase PWM in prose. Also the commit description is a
full sentence so it should start with a capital letter and end with a
full-stop.

Thierry

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

[PATCH V3 2/3] pwm: pxa: use module_platform_driver for driver register

[Chao Xie]

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 drivers/pwm/pwm-pxa.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index dee6ab55..aa4bea7 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -187,16 +187,6 @@ static struct platform_driver pwm_driver = {
 	.id_table	= pwm_id_table,
 };
 
-static int __init pwm_init(void)
-{
-	return platform_driver_register(&pwm_driver);
-}
-arch_initcall(pwm_init);
-
-static void __exit pwm_exit(void)
-{
-	platform_driver_unregister(&pwm_driver);
-}
-module_exit(pwm_exit);
+module_platform_driver(pwm_driver);
 
 MODULE_LICENSE("GPL v2");
-- 
1.7.4.1

Re: [PATCH V3 2/3] pwm: pxa: use module_platform_driver for driver register

[Thierry Reding]

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

On Tue, Apr 23, 2013 at 10:23:53PM -0400, Chao Xie wrote:
> Signed-off-by: Chao Xie <chao.xie@marvell.com>

The subject is a bit redundant. Something like:

	pwm: pxa: use module_platform_driver()

would be enough.

But I'd like to see a commit message which also mentions that the driver
used to be registered with arch_initcall() and why it is okay that it
isn't anymore.

Thierry

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

[PATCH V3 3/3] pwm: pxa: add device tree support

[Chao Xie]

Add the deice tree support for pwm-pxa.

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 drivers/pwm/pwm-pxa.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index aa4bea7..45d9047 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -9,6 +9,8 @@
  *
  * 2008-02-13	initial version
  * 		eric miao <eric.miao@marvell.com>
+ * 2013-04-24   add device tree support
+ * 		Chao Xie <chao.xie@marvell.com>
  */
 
 #include <linux/module.h>
@@ -18,6 +20,8 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/pwm.h>
 
 #include <asm/div64.h>
@@ -124,9 +128,52 @@ static struct pwm_ops pxa_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
-static int pwm_probe(struct platform_device *pdev)
+static struct of_device_id pxa_pwm_of_match[] = {
+	{
+		.compatible = "marvell,pxa25x-pwm",
+	}, {
+		.compatible = "marvell,pxa27x-pwm",
+		.data = (void *)HAS_SECONDARY_PWM
+	}, {
+		.compatible = "marvell,pxa168-pwm",
+	}, {
+		.compatible = "marvell,pxa910-pwm",
+	},
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, pxa_pwm_of_match);
+
+#ifdef CONFIG_OF
+static int pxa_pwm_parse_data(struct platform_device *pdev,
+				struct pxa_pwm_chip *pwm)
+{
+	const struct of_device_id *of_id =
+			of_match_device(pxa_pwm_of_match, &pdev->dev);
+	unsigned int npwm;
+
+	if (!of_id)
+		return -ENODEV;
+
+	npwm = (unsigned int)(of_id->data);
+	pwm->chip.npwm = (npwm & HAS_SECONDARY_PWM) ? 2 : 1;
+
+	return 0;
+}
+#else
+static int pxa_pwm_parse_data(struct platform_device *pdev,
+				struct pxa_pwm_chip *pwm)
 {
 	const struct platform_device_id *id = platform_get_device_id(pdev);
+
+	pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
+
+	return 0;
+}
+#endif
+
+static int pwm_probe(struct platform_device *pdev)
+{
 	struct pxa_pwm_chip *pwm;
 	struct resource *r;
 	int ret = 0;
@@ -144,7 +191,6 @@ static int pwm_probe(struct platform_device *pdev)
 	pwm->chip.dev = &pdev->dev;
 	pwm->chip.ops = &pxa_pwm_ops;
 	pwm->chip.base = -1;
-	pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (r == NULL) {
@@ -156,6 +202,12 @@ static int pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(pwm->mmio_base))
 		return PTR_ERR(pwm->mmio_base);
 
+	ret = pxa_pwm_parse_data(pdev, pwm);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to parse data from device id\n");
+		return ret;
+	}
+
 	ret = pwmchip_add(&pwm->chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
@@ -181,6 +233,7 @@ static struct platform_driver pwm_driver = {
 	.driver		= {
 		.name	= "pxa25x-pwm",
 		.owner	= THIS_MODULE,
+		.of_match_table	= pxa_pwm_of_match,
 	},
 	.probe		= pwm_probe,
 	.remove		= pwm_remove,
-- 
1.7.4.1

Re: [PATCH V3 3/3] pwm: pxa: add device tree support

[Thierry Reding]

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

On Tue, Apr 23, 2013 at 10:23:54PM -0400, Chao Xie wrote:
[...]
> @@ -124,9 +128,52 @@ static struct pwm_ops pxa_pwm_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> -static int pwm_probe(struct platform_device *pdev)
> +static struct of_device_id pxa_pwm_of_match[] = {

static const, please.

> +#ifdef CONFIG_OF
> +static int pxa_pwm_parse_data(struct platform_device *pdev,
> +				struct pxa_pwm_chip *pwm)
> +{
> +	const struct of_device_id *of_id =
> +			of_match_device(pxa_pwm_of_match, &pdev->dev);
> +	unsigned int npwm;
> +
> +	if (!of_id)
> +		return -ENODEV;
> +
> +	npwm = (unsigned int)(of_id->data);

The parentheses around of_id->data are unnecessary.

Thierry

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

Re: [PATCH V3 3/3] pwm: pxa: add device tree support

[Thierry Reding]

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

On Tue, Apr 23, 2013 at 10:23:54PM -0400, Chao Xie wrote:
> Add the deice tree support for pwm-pxa.

Instead of repeating the patch subject here, maybe you could be more
verbose about the changes you make, like splitting off the parsing of OF
and platform data into separate functions.

> diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
> index aa4bea7..45d9047 100644
> --- a/drivers/pwm/pwm-pxa.c
> +++ b/drivers/pwm/pwm-pxa.c
> @@ -9,6 +9,8 @@
>   *
>   * 2008-02-13	initial version
>   * 		eric miao <eric.miao@marvell.com>
> + * 2013-04-24   add device tree support
> + * 		Chao Xie <chao.xie@marvell.com>
>   */
>  
>  #include <linux/module.h>
> @@ -18,6 +20,8 @@
>  #include <linux/err.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/pwm.h>
>  
>  #include <asm/div64.h>
> @@ -124,9 +128,52 @@ static struct pwm_ops pxa_pwm_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> -static int pwm_probe(struct platform_device *pdev)
> +static struct of_device_id pxa_pwm_of_match[] = {
> +	{
> +		.compatible = "marvell,pxa25x-pwm",
> +	}, {
> +		.compatible = "marvell,pxa27x-pwm",
> +		.data = (void *)HAS_SECONDARY_PWM
> +	}, {
> +		.compatible = "marvell,pxa168-pwm",
> +	}, {
> +		.compatible = "marvell,pxa910-pwm",
> +	},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, pxa_pwm_of_match);
> +
> +#ifdef CONFIG_OF
> +static int pxa_pwm_parse_data(struct platform_device *pdev,
> +				struct pxa_pwm_chip *pwm)
> +{
> +	const struct of_device_id *of_id =
> +			of_match_device(pxa_pwm_of_match, &pdev->dev);
> +	unsigned int npwm;
> +
> +	if (!of_id)
> +		return -ENODEV;
> +
> +	npwm = (unsigned int)(of_id->data);
> +	pwm->chip.npwm = (npwm & HAS_SECONDARY_PWM) ? 2 : 1;
> +
> +	return 0;
> +}
> +#else
> +static int pxa_pwm_parse_data(struct platform_device *pdev,
> +				struct pxa_pwm_chip *pwm)
>  {
>  	const struct platform_device_id *id = platform_get_device_id(pdev);
> +
> +	pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
> +
> +	return 0;
> +}
> +#endif

One thing that occurred to me just now is that this might be a potential
problem. Suppose you want to build a kernel which has support for using
legacy board support (platform data) and DT. If you enable CONFIG_OF you
will no longer be able to support boards that use platform data.

A common solution to this problem is to prefer platform over DT data, so
typically you'd do something like this:

	if (!pdata) {
		if (IS_ENABLED(CONFIG_OF))
			err = parse_dt();
		else
			err = -ENODEV;
	} else {
		err = parse_pdata();
	}

	if (err < 0)
		return err;

Given that in your case you don't have platform data but rather the
platform device ID entry, the same scheme should work, since in the OF
case the id_entry of the platform device won't be set.

Thierry

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

Re: [PATCH V3 0/3] pwm: pxa: bug fix and device tree support

[Thierry Reding]

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

When you send the next version, can you please Cc Eric Miao. I'd like to
get his Acked-by on this series.

Thanks,
Thierry

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