| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120902195917.GB10930@avionic-0098.mockup.avionic-design.de>
Date: Sun, 2 Sep 2012 21:59:17 +0200
From: Thierry Reding <thierry.reding@...onic-design.de>
To: Lars-Peter Clausen <lars@...afoo.de>
Cc: Ralf Baechle <ralf@...ux-mips.org>, linux-mips@...ux-mips.org,
linux-kernel@...r.kernel.org,
Antony Pavlov <antonynpavlov@...il.com>,
Maarten ter Huurne <maarten@...ewalker.org>
Subject: Re: [PATCH 3/3] pwm: Add Ingenic JZ4740 support
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@...afoo.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
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists