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

[PATCH V4 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".

V4->V3
  Change comments for "pwm: pxa: use module_platform_driver()"
  and "pwm: pxa: ARCH_MMP share same pwm driver with ARCH_PXA".
  Use "static const" for pxa_pwm_of_match definition.
  Use the below scheme for DT support.
  	if (!id_entry) {
		if (IS_ENABLED(CONFIG_OF))
			err = parse_dt();
		else
			err = -ENODEV;
	  } else {
  		err = parse_pdata();
	  }

	  if (err < 0)
		  return err;

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

 drivers/pwm/Kconfig   |    2 +-
 drivers/pwm/pwm-pxa.c |   64 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 53 insertions(+), 13 deletions(-)

-- 
1.7.4.1

[PATCH V4 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 V4 1/3] pwm: pxa: ARCH_MMP share same pwm driver with ARCH_PXA

[Eric Miao]

On Mon, May 6, 2013 at 9:29 AM, Chao Xie <chao.xie@marvell.com> wrote:
> The PWM driver is not only used by ARCH_PXA but also ARCH_MMP.
>
> Signed-off-by: Chao Xie <chao.xie@marvell.com>

Looks good to me.

Acked-by: Eric Miao <eric.y.miao@gmail.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
>

[PATCH V4 2/3] pwm: pxa: use module_platform_driver()

[Chao Xie]

Old pwm-pxa.c will register driver by arch_initcall. Then other
drivers based on the PWM driver can successully call old
pwm_request because arch_initcall make sure the PWM driver will
be registered earlier.
Now, pwm_request is re-written and done by common layer code. It
will return -EPROBE_DEFER if the PWM device is not probed.
The driver based on PWM driver can make use of -EPROBE_DEFER
to delay its probing.
So arch_initcall can be replaced by module_platform_driver.

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 V4 2/3] pwm: pxa: use module_platform_driver()

[Eric Miao]

On Mon, May 6, 2013 at 9:30 AM, Chao Xie <chao.xie@marvell.com> wrote:
> Old pwm-pxa.c will register driver by arch_initcall. Then other
> drivers based on the PWM driver can successully call old
> pwm_request because arch_initcall make sure the PWM driver will
> be registered earlier.
> Now, pwm_request is re-written and done by common layer code. It
> will return -EPROBE_DEFER if the PWM device is not probed.
> The driver based on PWM driver can make use of -EPROBE_DEFER
> to delay its probing.
> So arch_initcall can be replaced by module_platform_driver.
>
> Signed-off-by: Chao Xie <chao.xie@marvell.com>

Looks good to me,

Acked-by: Eric Miao <eric.y.miao@gmail.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
>

[PATCH V4 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 |   52 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index aa4bea7..c8d59a2 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,6 +128,38 @@ static struct pwm_ops pxa_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+static const 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);
+
+static int pxa_pwm_parse_data_dt(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;
+}
+
 static int pwm_probe(struct platform_device *pdev)
 {
 	const struct platform_device_id *id = platform_get_device_id(pdev);
@@ -144,7 +180,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 +191,20 @@ static int pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(pwm->mmio_base))
 		return PTR_ERR(pwm->mmio_base);
 
+	if (!id) {
+		if (IS_ENABLED(CONFIG_OF))
+			ret = pxa_pwm_parse_data_dt(pdev, pwm);
+		else
+			ret = -ENODEV;
+	} else {
+		pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
+	}
+
+	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 +230,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 V4 3/3] pwm: pxa: add device tree support

[Eric Miao]

On Mon, May 6, 2013 at 9:30 AM, Chao Xie <chao.xie@marvell.com> wrote:
> Add the deice tree support for pwm-pxa.
>
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> ---
>  drivers/pwm/pwm-pxa.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 51 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
> index aa4bea7..c8d59a2 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,6 +128,38 @@ static struct pwm_ops pxa_pwm_ops = {
>         .owner = THIS_MODULE,
>  };
>
> +static const 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);
> +
> +static int pxa_pwm_parse_data_dt(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;
> +}
> +
>  static int pwm_probe(struct platform_device *pdev)
>  {
>         const struct platform_device_id *id = platform_get_device_id(pdev);
> @@ -144,7 +180,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 +191,20 @@ static int pwm_probe(struct platform_device *pdev)
>         if (IS_ERR(pwm->mmio_base))
>                 return PTR_ERR(pwm->mmio_base);
>
> +       if (!id) {
> +               if (IS_ENABLED(CONFIG_OF))
> +                       ret = pxa_pwm_parse_data_dt(pdev, pwm);
> +               else
> +                       ret = -ENODEV;
> +       } else {
> +               pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
> +       }

^^ braces not necessarily here, and I'm not really sure if we should check
CONFIG_OF firstly, and leave the platform_device_id thing as a legacy
fall back, what do you think?

  if (IS_ENABLED(CONFIG_OF)) {
     ...
  } else {
    const struct platform_device_id *id = platform_get_device_id(...);
    ...
  }

> +
> +       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 +230,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 V4 3/3] pwm: pxa: add device tree support

[Chao Xie]

>> +       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;
>> +}
>> +
>>  static int pwm_probe(struct platform_device *pdev)
>>  {
>>         const struct platform_device_id *id = platform_get_device_id(pdev);
>> @@ -144,7 +180,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 +191,20 @@ static int pwm_probe(struct platform_device *pdev)
>>         if (IS_ERR(pwm->mmio_base))
>>                 return PTR_ERR(pwm->mmio_base);
>>
>> +       if (!id) {
>> +               if (IS_ENABLED(CONFIG_OF))
>> +                       ret = pxa_pwm_parse_data_dt(pdev, pwm);
>> +               else
>> +                       ret = -ENODEV;
>> +       } else {
>> +               pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
>> +       }
>
> ^^ braces not necessarily here, and I'm not really sure if we should check
> CONFIG_OF firstly, and leave the platform_device_id thing as a legacy
> fall back, what do you think?
>
>   if (IS_ENABLED(CONFIG_OF)) {
>      ...
>   } else {
>     const struct platform_device_id *id = platform_get_device_id(...);
>     ...
>   }
>
it has some reasons.
You ways works for
     1. PWM defined in DT configuration file and CONFIG_OF is defined
     2. CONFIG_OF is not defined.
If COFNIG_OF is defined, but PWM is not defined in device tree
configuration file. The device
can still match the driver is the id_table is matched or name is matched.
So I covered addtional situation
    1. PWM is not defined in DT configuration file but CONFIG_OF is defined.

So i keep the possiblility that event CONFIG_OF is defined, but PWM is
not defined in DT file, and we still can use old way to probe the
device.

>> +
>> +       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 +230,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 V4 3/3] pwm: pxa: add device tree support

[Eric Miao]

On Mon, May 13, 2013 at 1:04 PM, Chao Xie <xiechao.mail@gmail.com> wrote:
>>> +       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;
>>> +}
>>> +
>>>  static int pwm_probe(struct platform_device *pdev)
>>>  {
>>>         const struct platform_device_id *id = platform_get_device_id(pdev);
>>> @@ -144,7 +180,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 +191,20 @@ static int pwm_probe(struct platform_device *pdev)
>>>         if (IS_ERR(pwm->mmio_base))
>>>                 return PTR_ERR(pwm->mmio_base);
>>>
>>> +       if (!id) {
>>> +               if (IS_ENABLED(CONFIG_OF))
>>> +                       ret = pxa_pwm_parse_data_dt(pdev, pwm);
>>> +               else
>>> +                       ret = -ENODEV;
>>> +       } else {
>>> +               pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
>>> +       }
>>
>> ^^ braces not necessarily here, and I'm not really sure if we should check
>> CONFIG_OF firstly, and leave the platform_device_id thing as a legacy
>> fall back, what do you think?
>>
>>   if (IS_ENABLED(CONFIG_OF)) {
>>      ...
>>   } else {
>>     const struct platform_device_id *id = platform_get_device_id(...);
>>     ...
>>   }
>>
> it has some reasons.
> You ways works for
>      1. PWM defined in DT configuration file and CONFIG_OF is defined
>      2. CONFIG_OF is not defined.
> If COFNIG_OF is defined, but PWM is not defined in device tree
> configuration file. The device
> can still match the driver is the id_table is matched or name is matched.
> So I covered addtional situation
>     1. PWM is not defined in DT configuration file but CONFIG_OF is defined.
>
> So i keep the possiblility that event CONFIG_OF is defined, but PWM is
> not defined in DT file, and we still can use old way to probe the
> device.
>

Yeah I was thinking maybe we should not keep the legacy working if
CONFIG_OF is defined, but that should be OK:

Acked-by: Eric Miao <eric.y.miao@gmail.com>

>>> +
>>> +       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 +230,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 V4 3/3] pwm: pxa: add device tree support

[Chao Xie]

hi, Thierry
Eric has acked all the patches. So can you help to merge them?
Thanks.


On Mon, May 13, 2013 at 2:20 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> On Mon, May 13, 2013 at 1:04 PM, Chao Xie <xiechao.mail@gmail.com> wrote:
>>>> +       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;
>>>> +}
>>>> +
>>>>  static int pwm_probe(struct platform_device *pdev)
>>>>  {
>>>>         const struct platform_device_id *id = platform_get_device_id(pdev);
>>>> @@ -144,7 +180,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 +191,20 @@ static int pwm_probe(struct platform_device *pdev)
>>>>         if (IS_ERR(pwm->mmio_base))
>>>>                 return PTR_ERR(pwm->mmio_base);
>>>>
>>>> +       if (!id) {
>>>> +               if (IS_ENABLED(CONFIG_OF))
>>>> +                       ret = pxa_pwm_parse_data_dt(pdev, pwm);
>>>> +               else
>>>> +                       ret = -ENODEV;
>>>> +       } else {
>>>> +               pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
>>>> +       }
>>>
>>> ^^ braces not necessarily here, and I'm not really sure if we should check
>>> CONFIG_OF firstly, and leave the platform_device_id thing as a legacy
>>> fall back, what do you think?
>>>
>>>   if (IS_ENABLED(CONFIG_OF)) {
>>>      ...
>>>   } else {
>>>     const struct platform_device_id *id = platform_get_device_id(...);
>>>     ...
>>>   }
>>>
>> it has some reasons.
>> You ways works for
>>      1. PWM defined in DT configuration file and CONFIG_OF is defined
>>      2. CONFIG_OF is not defined.
>> If COFNIG_OF is defined, but PWM is not defined in device tree
>> configuration file. The device
>> can still match the driver is the id_table is matched or name is matched.
>> So I covered addtional situation
>>     1. PWM is not defined in DT configuration file but CONFIG_OF is defined.
>>
>> So i keep the possiblility that event CONFIG_OF is defined, but PWM is
>> not defined in DT file, and we still can use old way to probe the
>> device.
>>
>
> Yeah I was thinking maybe we should not keep the legacy working if
> CONFIG_OF is defined, but that should be OK:
>
> Acked-by: Eric Miao <eric.y.miao@gmail.com>
>
>>>> +
>>>> +       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 +230,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 V4 3/3] pwm: pxa: add device tree support

[Thierry Reding]

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

On Tue, May 21, 2013 at 01:21:29PM +0800, Chao Xie wrote:
> hi, Thierry
> Eric has acked all the patches. So can you help to merge them?
> Thanks.

I'm currently in the process of migrating everything to my private email
address so things are a bit bumpy and I'm lagging behind. I have them in
my queue, though, and should be able to apply and push them sometime
soon.

Thierry

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