linux-kernel - Re: [PATCH 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] [day] [month] [year] [list]

Message-ID: <20130418080329.GA18212@avionic-0098.mockup.avionic-design.de>
Date:	Thu, 18 Apr 2013 10:03:29 +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 3/3] pwm: pxa: add device tree support

On Tue, Apr 16, 2013 at 02:51:55AM -0400, Chao Xie wrote:
> Add the deice tree support for pwm-pxa.

"device" and you can drop "the".

>  drivers/pwm/pwm-pxa.c |   31 ++++++++++++++++++++++++++++++-
>  1 files changed, 30 insertions(+), 1 deletions(-)

This is missing a file in Documentation/devicetree/bindings/pwm that
describe the device tree bindings for this driver.

> diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
[...]
> +static struct of_device_id pxa_pwm_of_match[] = {

I know not all drivers do this, but this should be "static const".

> +	{
> +		.compatible = "mrvl,pxa25x-pwm",

According to Documentation/devicetree/bindings/vendor-prefixes.txt the
vendor prefix for Marvell is "marvell".

> +		.data = (void *)0

Initialization to 0 is redundant here.

>  static int pwm_probe(struct platform_device *pdev)
>  {
>  	const struct platform_device_id *id = platform_get_device_id(pdev);
> +	const struct of_device_id *of_id =
> +			of_match_device(pxa_pwm_of_match, &pdev->dev);
>  	struct pxa_pwm_chip *pwm;
>  	struct resource *r;
>  	int ret = 0;
> +	unsigned int npwm;
>  
>  	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
>  	if (pwm == NULL) {
> @@ -157,7 +181,12 @@ 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;
> +	if (of_id)
> +		npwm = (unsigned int)(of_id->data);
> +	else
> +		npwm = id->driver_data;
> +
> +	pwm->chip.npwm = (npwm & HAS_SECONDARY_PWM) ? 2 : 1;

Can you rebase this on my for-next branch since it has some patches that
this conflicts with.

Also I think this patch is missing the assignment of

	.of_match_table = pxa_pwm_of_match;

in the platform_driver structure initialization. And I think PXA hasn't
been fully converted to OF-only, so you need to conditionalize the OF
bits.

Thierry

Content of type "application/pgp-signature" skipped