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 22:37:22 +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: <5043C005.8060907@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> <20120902195917.GB10930@avionic-0098.mockup.avionic-design.de> <5043C005.8060907@metafoo.de>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
On Sun, Sep 02, 2012 at 10:22:29PM +0200, Lars-Peter Clausen wrote:
> On 09/02/2012 09:59 PM, Thierry Reding wrote:
> >>> + 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?
> >
> 
> It can, but we've observed this to cause permanent glitches until the timer is
> reprogrammed again.

Okay. I've changed this to use jz4740_pwm_{enable,disable}() instead.

> >>> +{
> >>> + 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.
> 
> Part of the Linux device driver model is that that a device may appear or
> disappear at any given time (if the kernel has been compiled with
> CONFIG_HOTPLUG). So you can't prevent removal. The fact that the remove
> callback function return an int is kind of misleading and should probably be
> fixed at some point. The return value is never checked and the device will be
> removed nevertheless. So the PWM subsystem must cope with the case where the
> PWM chip is removed while some of its pwm_devices are still in use.

I thought I had seen this work. But looking at the code, you're right.
Perhaps what I saw was caused by the reference counting done on the
pwm_ops structure. At least that keeps the module from being unloaded if
there are still any requested PWM devices, but it won't help if the
device suddenly goes away. I wonder if that's a realistic use-case,
though, at least for platform devices.

I currently can't run any tests because I don't have any hardware
available. I'll need to take another look when I'm back at work next
week and think of a way to solve this. Adding some reference counting as
you suggested earlier may be the only way.

Thierry

Attachment: pgppZ2dZT1HNT.pgp
Description: PGP signature

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