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

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 3095 bytes --]

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 #2: Type: application/pgp-signature, Size: 836 bytes --]