On Sun, Sep 02, 2012 at 10:22:29PM +0200, Lars-Peter Clausen wrote:

On 09/02/2012 09:59 PM, Thierry Reding wrote:

quoted

quoted

quoted

+	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
+	if (is_enabled)
+		pwm_disable(pwm);

I think this should be jz4740_pwm_disable

quoted

+
+	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.

quoted

quoted

quoted

+{
+	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