linux-kernel - Re: [PATCH] pwm: add ep93xx PWM 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: <20131015103932.GN7856@ulmo.nvidia.com>
Date:	Tue, 15 Oct 2013 12:39:33 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	H Hartley Sweeten <hartleys@...ionengravers.com>
Cc:	linux-pwm@...r.kernel.org,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Ryan Mallon <rmallon@...il.com>, arnd@...db.de,
	gregkh@...uxfoundation.org
Subject: Re: [PATCH] pwm: add ep93xx PWM support

On Mon, Oct 14, 2013 at 02:57:48PM -0700, H Hartley Sweeten wrote:
> Remove the non-standard EP93xx pwm driver in drivers/misc and add

pwm -> PWM

> a new driver for the PWM chips on the EP93xx platforms based on the
> PWM framework.
> 
> These PWM chips each support 1 PWM channel with programmable duty

Perhaps "chips" -> "controllers"?

> cycle, frequency, and polarity inversion.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten@...ionengravers.com>
> Cc: Ryan Mallon <rmallon@...il.com>
> Cc: Thierry Reding <thierry.reding@...il.com>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> 
> diff --git a/drivers/misc/ep93xx_pwm.c b/drivers/misc/ep93xx_pwm.c
[...]
> - *	(c) Copyright 2009  Matthieu Crapet <mcrapet@...il.com>
> - *	(c) Copyright 2009  H Hartley Sweeten <hsweeten@...ionengravers.com>
[...]
> -MODULE_AUTHOR("Matthieu Crapet <mcrapet@...il.com>, "
> -	      "H Hartley Sweeten <hsweeten@...ionengravers.com>");
[...]

> diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c
[...]
> + * Copyright (C) 2013 H Hartley Sweeten <hsweeten@...ionengravers.com>
[...]
> +MODULE_AUTHOR("H Hartley Sweeten <hsweeten@...ionengravers.com>");

Why are you removing Matthieu from the list of authors and copyright
here? From a brief look it seems like this new driver is still based on
code from the old driver and not a complete rewrite.

> +#include <mach/platform.h>	/* for ep93xx_pwm_{acquire,release}_gpio() */

I'm not sure how well that will play together with multiplatform support
but perhaps that's not an issue for ep93xx?

> +static int ep93xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct platform_device *pdev = to_platform_device(chip->dev);
> +
> +	return ep93xx_pwm_acquire_gpio(pdev);
> +}
> +
> +static void ep93xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct platform_device *pdev = to_platform_device(chip->dev);
> +
> +	ep93xx_pwm_release_gpio(pdev);
> +}

This looks like it would belong in the domain of pinctrl, but I suspect
that ep93xx doesn't support that.

> +static int ep93xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     int duty_ns, int period_ns)
> +{
> +	struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
> +	void __iomem *base = ep93xx_pwm->base;
> +	unsigned long long c;
> +	unsigned long period_cycles;
> +	unsigned long duty_cycles;
> +	unsigned long term;
> +	int ret = 0;
> +
> +	/*
> +	 * The clock needs to be enabled to access the PWM registers.
> +	 * Configuration can be changed at any time.
> +	 */
> +	if (!test_bit(PWMF_ENABLED, &pwm->flags))
> +		clk_enable(ep93xx_pwm->clk);

clk_enable() can fail, so you should check the return value and
propagate errors.

> +static int ep93xx_pwm_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +			       enum pwm_polarity polarity)
> +{
> +	struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
> +
> +	/*
> +	 * The clock needs to be enabled to access the PWM registers.
> +	 * Polarity can only be changed when the PWM is disabled.
> +	  */

Nit: the closing */ is wrongly aligned.

> +	clk_enable(ep93xx_pwm->clk);

Needs a check of the return value.

> +	writew(polarity, ep93xx_pwm->base + EP93XX_PWMx_INVERT);

I'd prefer if this did some explicit conversion from the PWM framework
value to the driver-specific value, even if they happen to be the same
in this case.

> +static int ep93xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
> +
> +	clk_enable(ep93xx_pwm->clk);

Also needs to check the return value.

> +static struct pwm_ops ep93xx_pwm_ops = {

static const, please.

> +static int ep93xx_pwm_remove(struct platform_device *pdev)
> +{
> +	struct ep93xx_pwm *ep93xx_pwm;
> +
> +	ep93xx_pwm = platform_get_drvdata(pdev);
> +	if (!ep93xx_pwm)
> +		return -ENODEV;

No need for this check. It will never happen.

> +
> +	return pwmchip_remove(&ep93xx_pwm->chip);
> +}
> +
> +static struct platform_driver ep93xx_pwm_driver = {
> +	.driver		= {
> +		.name	= "ep93xx-pwm",
> +		.owner	= THIS_MODULE,

This is no longer required because the core sets it to the proper value.

> +	},
> +	.probe		= ep93xx_pwm_probe,
> +	.remove		= ep93xx_pwm_remove,
> +};

Oh, and I didn't mention it before, but please get rid of all the
needless tabs for alignment. It's completely useless and doesn't help
with readability at all in my opinion.

Thierry

Content of type "application/pgp-signature" skipped