Re: [PATCH 3/3] pwm: Add Ingenic JZ4740 support

[Thierry Reding]

On Sun, Sep 02, 2012 at 04:44:15PM +0200, Lars-Peter Clausen wrote:
> On 09/02/2012 11:52 AM, Thierry Reding wrote:
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 92b1782..f5acdaa 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -371,7 +371,7 @@ EXPORT_SYMBOL_GPL(pwm_free);
> >   */
> >  int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> > -	if (!pwm || period_ns == 0 || duty_ns > period_ns)
> > +	if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
> >  		return -EINVAL;
> >  
> 
> This change seems to be unrelated.

Yes, that slipped in by mistake. That was supposed to go into a separate
patch so that the .config of each driver doesn't have to repeat these
checks.

> >  	return pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> > diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> > new file mode 100644
> > index 0000000..db29b37
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-jz4740.c
> > @@ -0,0 +1,205 @@
> > +/*
> > + *  Copyright (C) 2010, Lars-Peter Clausen <lars@xxxxxxxxxx>
> > + *  JZ4740 platform PWM support
> > + *
> > + *  This program is free software; you can redistribute it and/or modify it
> > + *  under  the terms of the GNU General  Public License as published by the
> > + *  Free Software Foundation;  either version 2 of the License, or (at your
> > + *  option) any later version.
> > + *
> > + *  You should have received a copy of the GNU General Public License along
> > + *  with this program; if not, write to the Free Software Foundation, Inc.,
> > + *  675 Mass Ave, Cambridge, MA 02139, USA.
> > + *
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +
> > +#include <asm/mach-jz4740/gpio.h>
> > +#include <timer.h>
> 
> #include <asm/mach-jz4740/timer.h>
> 
> > +
> > +#define NUM_PWM 6
> > +
> > +static const unsigned int jz4740_pwm_gpio_list[NUM_PWM] = {
> 
> As mth said, it would be better to have JZ_GPIO_PWM0 and here as well and set
> NUM_PWM to 8. Right now we are using the timers associated to PWM channel 0 and
> 1 as system timers. But there might be devices where this is not possible, e.g.
> because the PWM is actually connected to something. Also this fixes the of by
> two for the hwpwm id.

Okay. I was actually planning on doing some cleanup in a follow-up patch
and try to limit actual changes in this patch to what is required by the
conversion. However if Maarten and you both are okay with it I can make
these additional changes while at it.

> > +	if (ret) {
> > +		printk(KERN_ERR "Failed to request pwm gpio: %d\n", ret);
> 
> dev_err(chip->dev, ....

Okay.

> > +	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
> > +	if (is_enabled)
> > +		pwm_disable(pwm);
> 
> I think this should be jz4740_pwm_disable
> 
> > +
> > +	jz4740_timer_set_count(pwm->hwpwm, 0);
> > +	jz4740_timer_set_duty(pwm->hwpwm, duty);
> > +	jz4740_timer_set_period(pwm->hwpwm, period);
> > +
> > +	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
> > +		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
> > +
> > +	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
> > +
> > +	if (is_enabled)
> > +		pwm_enable(pwm);
> 
> and jz4740_pwm_enable here.

I wonder if this is actually required here. Can the timer really not be
reprogrammed while enabled?

> > +
> > +	return 0;
> > +}
> > +
> 
> > +
> > +static const struct pwm_ops jz4740_pwm_ops = {
> > +	.request = jz4740_pwm_request,
> > +	.free = jz4740_pwm_free,
> > +	.config = jz4740_pwm_config,
> > +	.enable = jz4740_pwm_enable,
> > +	.disable = jz4740_pwm_disable,
> 
> .owner = THIS_MODULE,

Yes, I forgot that one.

> > +};
> > +
> > +static int jz4740_pwm_probe(struct platform_device *pdev)
> __devinit

Yes, I'll add that.

> > +{
> > +	struct jz4740_pwm_chip *jz4740;
> > +	int ret = 0;
> 
> The '= 0' is not really necessary since it will be overwritten anyway.

Right, I'll drop it.

> > +static int jz4740_pwm_remove(struct platform_device *pdev)
> __devexit

Can do.

> > +{
> > +	struct jz4740_pwm_chip *jz4740 = platform_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	ret = pwmchip_remove(&jz4740->chip);
> > +	if (ret < 0)
> > +		return ret;
> 
> remove is not really allowed to fail, the return value is never really tested
> and the device is removed nevertheless. But this seems to be a problem with the
> PWM API. It should be possible to remove a PWM chip even if it is currently in
> use and after a PWM chip has been removed all calls to a pwm_device of that
> chip it should return an error. This will require reference counting for the
> pwm_device struct though. E.g. by adding a 'struct device' to it.

I beg to differ. It shouldn't be possible to remove a PWM chip that
provides requested PWM devices. All other drivers do the same here.

> > +
> > +	clk_put(jz4740->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver jz4740_pwm_driver = {
> > +	.driver = {
> > +		.name = "jz4740-pwm",
> 
> .owner = THIS_MODULE,

There are a number of other drivers where this is missing. I'll make a
note to add it to those as well.

> > +	},
> > +	.probe = jz4740_pwm_probe,
> > +	.remove = jz4740_pwm_remove,
> 
> .remove = __devexit_p(jz4740_pwm_remove),

Yes.

> 
> > +};
> > +module_platform_driver(jz4740_pwm_driver);
> 
> MODULE_LICENSE(...), MODULE_AUTHOR(...), MODULE_DESCRIPTION(...), MODULE_ALIAS(...)

Those weren't present previously. I suppose they should be "GPL", you,
"Ingenic JZ4740 PWM driver" and "platform:jz4740-pwm", respectively?

Thierry

Attachment: pgp00000.pgp
Description: PGP signature