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

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- 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 --]