lkml.org 
[lkml]   [2013]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@visionengravers.com>
> Cc: Ryan Mallon <rmallon@gmail.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> diff --git a/drivers/misc/ep93xx_pwm.c b/drivers/misc/ep93xx_pwm.c
[...]
> - * (c) Copyright 2009 Matthieu Crapet <mcrapet@gmail.com>
> - * (c) Copyright 2009 H Hartley Sweeten <hsweeten@visionengravers.com>
[...]
> -MODULE_AUTHOR("Matthieu Crapet <mcrapet@gmail.com>, "
> - "H Hartley Sweeten <hsweeten@visionengravers.com>");
[...]

> diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c
[...]
> + * Copyright (C) 2013 H Hartley Sweeten <hsweeten@visionengravers.com>
[...]
> +MODULE_AUTHOR("H Hartley Sweeten <hsweeten@visionengravers.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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2013-10-15 13:21    [W:0.050 / U:0.864 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site