Date:	Wed, 16 Oct 2013 01:22:07 +0000
From:	Hartley Sweeten <HartleyS@...ionengravers.com>
To:	Thierry Reding <thierry.reding@...il.com>
CC:	"linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Ryan Mallon <rmallon@...il.com>,
	"arnd@...db.de" <arnd@...db.de>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Subject: RE: [PATCH] pwm: add ep93xx PWM support

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 <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.

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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists