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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130424073350.GG1429@avionic-0098.mockup.avionic-design.de>
Date:	Wed, 24 Apr 2013 09:33:50 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Chao Xie <chao.xie@...vell.com>
Cc:	linux-kernel@...r.kernel.org, xiechao.mail@...il.com
Subject: Re: [PATCH V3 3/3] pwm: pxa: add device tree support

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@...vell.com>
> + * 2013-04-24   add device tree support
> + * 		Chao Xie <chao.xie@...vell.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

Content of type "application/pgp-signature" skipped