On Tuesday, October 15, 2013 3:40 AM, Thierry Reding wrote: > 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
OK >> 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"? OK >> cycle, frequency, and polarity inversion. >> >> Signed-off-by: H Hartley Sweeten <[email protected]> >> Cc: Ryan Mallon <[email protected]> >> Cc: Thierry Reding <[email protected]> >> Cc: Arnd Bergmann <[email protected]> >> Cc: Greg Kroah-Hartman <[email protected]> >> >> diff --git a/drivers/misc/ep93xx_pwm.c b/drivers/misc/ep93xx_pwm.c > [...] >> - * (c) Copyright 2009 Matthieu Crapet <[email protected]> >> - * (c) Copyright 2009 H Hartley Sweeten <[email protected]> > [...] >> -MODULE_AUTHOR("Matthieu Crapet <[email protected]>, " >> - "H Hartley Sweeten <[email protected]>"); > [...] > >> diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c > [...] >> + * Copyright (C) 2013 H Hartley Sweeten <[email protected]> > [...] >> +MODULE_AUTHOR("H Hartley Sweeten <[email protected]>"); > > 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. My bad. It is based on the misc driver but I forgot to put Matthieu in as one of the original authors when I wrote it. I'll fix that. >> +#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? For multiplatform it would probably be a problem. But I don't think anyone would be including ep93xx in a multiplatform kernel. If the problem comes up I'll figure out some way to deal with it, probably with a pinctrl driver. >> +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. It should be but I have not worked out how to support EP93xx GPIOs with a pinctrl driver yet. The GPIOs are pretty limited on this platform compared to the other pinctrl users. >> +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. I overlooked that. This will be fixed in the next version. >> +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. OK >> + clk_enable(ep93xx_pwm->clk); > > Needs a check of the return value. OK >> + 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. OK >> +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. OK >> +static struct pwm_ops ep93xx_pwm_ops = { > > static const, please. OK >> +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. OK >> + >> + 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. OK >> + }, >> + .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. Opinions differ.. But I'll remove the tabs. Thanks for the review, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
