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
Add the deice tree support for pwm-pxa.
Signed-off-by: Chao Xie <[email protected]>
---
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 <[email protected]>
+ * 2013-04-24 add device tree support
+ * Chao Xie <[email protected]>
*/
#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
Signed-off-by: Chao Xie <[email protected]>
---
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
the pwm driver is not only used by ARCH_PXA but also ARCH_MMP
Signed-off-by: Chao Xie <[email protected]>
---
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
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
On Tue, Apr 23, 2013 at 10:23:53PM -0400, Chao Xie wrote:
> Signed-off-by: Chao Xie <[email protected]>
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
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
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 <[email protected]>
> + * 2013-04-24 add device tree support
> + * Chao Xie <[email protected]>
> */
>
> #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
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