Messages in this thread | | | Date | Tue, 15 Oct 2013 12:39:33 +0200 | From | Thierry Reding <> | 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@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] | |