linux-mips
[Top] [All Lists]

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

To: Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH 3/3] pwm: Add Ingenic JZ4740 support
From: Thierry Reding <thierry.reding@avionic-design.de>
Date: Sun, 2 Sep 2012 21:59:17 +0200
Cc: Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org, linux-kernel@vger.kernel.org, Antony Pavlov <antonynpavlov@gmail.com>, Maarten ter Huurne <maarten@treewalker.org>
In-reply-to: <504370BF.6090702@metafoo.de>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
References: <1346579550-5990-1-git-send-email-thierry.reding@avionic-design.de> <1346579550-5990-4-git-send-email-thierry.reding@avionic-design.de> <504370BF.6090702@metafoo.de>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
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@metafoo.de>
> > + *  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: pgpWxD9_7AovY.pgp
Description: PGP signature

<Prev in Thread] Current Thread [Next in Thread>