Skip to contentSkip to conversation

is pwm_put supposed to stop a PWM?

PATCH New Uwe Kleine-König 3 participants 17 messages Oct 25, 2018 – Nov 20, 2018

Subsystems (2)
PWM Arm/LPC18XX architecture
lore
Share & tools
lore

Uwe Kleine-König

author
#0 lore raw

Hello,

while digging around in the pwm drivers I found drivers/pwm/pwm-lpc18xx-sct.c which in its .free callback calls pwm_disable() and pwm_set_duty_cycle(pwm, 0).

I think calling pwm_disable is wrong for two reasons:

a) pwm_disable is for PWM users, not for drivers
b) .free isn't supposed to stop the pwm.

Also I'm surprised that .request calls pwm_get_duty_cycle() and writes the result into hardware. Still more as I think pwm_get_duty_cycle() always returns 0 in .request.

BTW, this is the only caller of pwm_set_duty_cycle(), so if the driver is simplified/fixed to not care about the duty cycle in .free and .request, this function can go away.

What do you think?

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#1 lore raw
Quoted 9 lines · Uwe Kleine-König

Hello,

while digging around in the pwm drivers I found drivers/pwm/pwm-lpc18xx-sct.c which in its .free callback calls pwm_disable() and pwm_set_duty_cycle(pwm, 0).

I think calling pwm_disable is wrong for two reasons:

a) pwm_disable is for PWM users, not for drivers

PWM controller drivers can still call pwm_disable(), though they should do so very carefully and not behind consumers' backs.

Quoted 1 line

b) .free isn't supposed to stop the pwm.

That's a bit of a gray area. I still think we need a way of stopping the PWM at some point. Some drivers do this on ->remove() and it was in fact discussed at one point that this should be moved to the core, so that all channels would be automatically disabled when the PWM chip is removed.

Disabling in ->free() should not have any effect, because users should already have disabled it before they even call pwm_put() on it.

So perhaps a good compromise would be to document that users are supposed to disable the PWM before they release it and then drivers (or the core) could go and check that they're really disabled when the chip is being removed and WARN() if not. That way we can flag all consumers that don't behave and fix them.

Quoted 9 lines

Also I'm surprised that .request calls pwm_get_duty_cycle() and writes the result into hardware. Still more as I think pwm_get_duty_cycle() always returns 0 in .request.

BTW, this is the only caller of pwm_set_duty_cycle(), so if the driver is simplified/fixed to not care about the duty cycle in .free and .request, this function can go away.

What do you think?

Yeah, it looks as if pwm_get_duty_cycle() would always return 0 because the driver doesn't implement hardware readout. pwm_set_duty_cycle() was introduced back at the time when sysfs support was added, but it seems to have otherwise been unused ever since. I think it no longer makes sense to keep it around once all consumers have adopted the atomic API.

Thierry

author
#2 lore raw

Hello Thierry,

Quoted 11 lines · Thierry Reding
Quoted 7 lines · Uwe Kleine-König

while digging around in the pwm drivers I found drivers/pwm/pwm-lpc18xx-sct.c which in its .free callback calls pwm_disable() and pwm_set_duty_cycle(pwm, 0).

I think calling pwm_disable is wrong for two reasons:

a) pwm_disable is for PWM users, not for drivers

PWM controller drivers can still call pwm_disable(), though they should do so very carefully and not behind consumers' backs.

I'd say this is a bad idea because the driver knows in which callback this results and so there is complexity for no real benefit. When the core implemented some locking this could easily deadlock.

Quoted 10 lines
Quoted 1 line

b) .free isn't supposed to stop the pwm.

That's a bit of a gray area. I still think we need a way of stopping the PWM at some point. Some drivers do this on ->remove() and it was in fact discussed at one point that this should be moved to the core, so that all channels would be automatically disabled when the PWM chip is removed.

Disabling in ->free() should not have any effect, because users should already have disabled it before they even call pwm_put() on it.

Then I recommend to remove this from the driver. There is no benefit and so it's code that is "dead", a bad example to copy from and keeping the code more complex than needed.

Quoted 5 lines

So perhaps a good compromise would be to document that users are supposed to disable the PWM before they release it and then drivers (or the core) could go and check that they're really disabled when the chip is being removed and WARN() if not. That way we can flag all consumers that don't behave and fix them.

Yeah, I'd say documenting is a good idea, too. And then check in the core (and WARN there) and remove the code from hardware drivers.

Quoted 8 lines
Quoted 8 lines

Also I'm surprised that .request calls pwm_get_duty_cycle() and writes the result into hardware. Still more as I think pwm_get_duty_cycle() always returns 0 in .request.

BTW, this is the only caller of pwm_set_duty_cycle(), so if the driver is simplified/fixed to not care about the duty cycle in .free and .request, this function can go away.

Quoted 7 lines
Quoted 1 line

What do you think?

Yeah, it looks as if pwm_get_duty_cycle() would always return 0 because the driver doesn't implement hardware readout. pwm_set_duty_cycle() was introduced back at the time when sysfs support was added, but it seems to have otherwise been unused ever since. I think it no longer makes sense to keep it around once all consumers have adopted the atomic API.

OK, will prepare a patch when I come around.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
author
#3 lore raw

Hello Thierry,

Quoted 8 lines · Uwe Kleine-König
Quoted 7 lines · Thierry Reding
Quoted 6 lines · Uwe Kleine-König

while digging around in the pwm drivers I found drivers/pwm/pwm-lpc18xx-sct.c which in its .free callback calls pwm_disable() and pwm_set_duty_cycle(pwm, 0).

I think calling pwm_disable is wrong for two reasons:

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

a) pwm_disable is for PWM users, not for drivers

PWM controller drivers can still call pwm_disable(), though they should do so very carefully and not behind consumers' backs.

I'd say this is a bad idea because the driver knows in which callback this results and so there is complexity for no real benefit. When the core implemented some locking this could easily deadlock.

I think it would be beneficial to get rid of these calls. Would you agree to document calling pwm-API calls as forbidden as a first step and then work on fixing these?

Quoted 15 lines
Quoted 10 lines
Quoted 1 line

b) .free isn't supposed to stop the pwm.

That's a bit of a gray area. I still think we need a way of stopping the PWM at some point. Some drivers do this on ->remove() and it was in fact discussed at one point that this should be moved to the core, so that all channels would be automatically disabled when the PWM chip is removed.

Disabling in ->free() should not have any effect, because users should already have disabled it before they even call pwm_put() on it.

Then I recommend to remove this from the driver. There is no benefit and so it's code that is "dead", a bad example to copy from and keeping the code more complex than needed.

Quoted 8 lines
Quoted 5 lines

So perhaps a good compromise would be to document that users are supposed to disable the PWM before they release it and then drivers (or the core) could go and check that they're really disabled when the chip is being removed and WARN() if not. That way we can flag all consumers that don't behave and fix them.

Yeah, I'd say documenting is a good idea, too. And then check in the core (and WARN there) and remove the code from hardware drivers.

Thinking about this, I'd not require that a consumer disables a pwm before calling pwm_put. Eventually it might have good reasons to keep the PWM running (e.g. to keep a led in its state). So I'd restrict the documentation to the undisputed part that the hw driver should only touch the configuration in .apply (respectively .config, .set_polarity, .enable, .disable for legacy drivers).

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#4 lore raw
Quoted 16 lines · Uwe Kleine-König

Hello Thierry,

Quoted 13 lines · Uwe Kleine-König
Quoted 11 lines · Thierry Reding
Quoted 7 lines · Uwe Kleine-König

while digging around in the pwm drivers I found drivers/pwm/pwm-lpc18xx-sct.c which in its .free callback calls pwm_disable() and pwm_set_duty_cycle(pwm, 0).

I think calling pwm_disable is wrong for two reasons:

a) pwm_disable is for PWM users, not for drivers

PWM controller drivers can still call pwm_disable(), though they should do so very carefully and not behind consumers' backs.

Quoted 7 lines
Quoted 3 lines

I'd say this is a bad idea because the driver knows in which callback this results and so there is complexity for no real benefit. When the core implemented some locking this could easily deadlock.

I think it would be beneficial to get rid of these calls. Would you agree to document calling pwm-API calls as forbidden as a first step and then work on fixing these?

I'm not a big fan of documenting anything as being forbidden. People can do whatever they want when nobody's checking and we don't have a way of enforcing anything either because they could just as well remove those enforcement measures. Remember, they have access to all the code, so the best we can do is discourage people from doing potentially stupid things by documenting expectations, requirements and limitations.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line

b) .free isn't supposed to stop the pwm.

That's a bit of a gray area. I still think we need a way of stopping the PWM at some point. Some drivers do this on ->remove() and it was in fact discussed at one point that this should be moved to the core, so that all channels would be automatically disabled when the PWM chip is removed.

Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

Disabling in ->free() should not have any effect, because users should already have disabled it before they even call pwm_put() on it.

Then I recommend to remove this from the driver. There is no benefit and so it's code that is "dead", a bad example to copy from and keeping the code more complex than needed.

Yeah, that's fair.

Quoted 9 lines
Quoted 8 lines
Quoted 5 lines

So perhaps a good compromise would be to document that users are supposed to disable the PWM before they release it and then drivers (or the core) could go and check that they're really disabled when the chip is being removed and WARN() if not. That way we can flag all consumers that don't behave and fix them.

Yeah, I'd say documenting is a good idea, too. And then check in the core (and WARN there) and remove the code from hardware drivers.

Quoted 6 lines

Thinking about this, I'd not require that a consumer disables a pwm before calling pwm_put. Eventually it might have good reasons to keep the PWM running (e.g. to keep a led in its state). So I'd restrict the documentation to the undisputed part that the hw driver should only touch the configuration in .apply (respectively .config, .set_polarity, .enable, .disable for legacy drivers).

What would you consider good reasons for keeping a PWM running when the consumer no longer needs it? I can't come up with any sane reason why we would want to allow that. It's normal to turn things off when you no longer need them. I do that with a computer, an oven and a shower, too. Why would a PWM be different?

Your example of keeping an LED in the current state is actually an example of where the consumer still needs it. As long as you want to use the LED you need to keep the LED driver around, and as long as the LED driver is around you have a consumer for the PWM. When the LED driver is unloaded, it better turn off that LED. Of course if the LED is supposed to be on by default, then most likely it will not be driven by a PWM in the first place, or it would be an inversed polarity PWM (driven by a pin that has a pull-up by default), in which case the right thing to do in the LED driver's ->remove() implementation is to disable the PWM and restore the pull-up on the pin to pull it high while it is not actively driven.

Thierry

author
#5 lore raw

Hello Thierry,

Quoted 23 lines · Thierry Reding
Quoted 14 lines · Uwe Kleine-König
Quoted 13 lines · Uwe Kleine-König
Quoted 11 lines · Thierry Reding
Quoted 7 lines · Uwe Kleine-König

while digging around in the pwm drivers I found drivers/pwm/pwm-lpc18xx-sct.c which in its .free callback calls pwm_disable() and pwm_set_duty_cycle(pwm, 0).

I think calling pwm_disable is wrong for two reasons:

a) pwm_disable is for PWM users, not for drivers

PWM controller drivers can still call pwm_disable(), though they should do so very carefully and not behind consumers' backs.

Quoted 7 lines
Quoted 3 lines

I'd say this is a bad idea because the driver knows in which callback this results and so there is complexity for no real benefit. When the core implemented some locking this could easily deadlock.

I think it would be beneficial to get rid of these calls. Would you agree to document calling pwm-API calls as forbidden as a first step and then work on fixing these?

Quoted 6 lines

I'm not a big fan of documenting anything as being forbidden. People can do whatever they want when nobody's checking and we don't have a way of enforcing anything either because they could just as well remove those enforcement measures. Remember, they have access to all the code, so the best we can do is discourage people from doing potentially stupid things by documenting expectations, requirements and limitations.

In my eyes the purpose of the PWM framework (and similar all other frameworks, too) is to abstract a common set of functions for a certain type of hardware. As such it is beneficial to all affected parties if the hardware drivers have a tight policy what is expected. This makes implementing stuff in the hardware driver easier, because people know what should be done and what not in a given callback. It makes pwm consumers' work easier, because they know what to expect from calling a certain API function and so increases the chances that a backlight driver that was only tested with say a rockchip PWM also works the same way when another machine makes use of the same backlight driver with an imx PWM.
This increases overall code quality.

Sure we cannot protect people from doing stupid things by patching stuff. But we can improve the situation for people who have to patch to actually fix broken stuff by documenting for them in the above scenario where the backlight driver doesn't work with the imx pwm if it's the backlight driver or the pwm driver that needs adaption to converge to a state where all pwm consumers work with all pwm backends.

Quoted 16 lines
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines

So perhaps a good compromise would be to document that users are supposed to disable the PWM before they release it and then drivers (or the core) could go and check that they're really disabled when the chip is being removed and WARN() if not. That way we can flag all consumers that don't behave and fix them.

Yeah, I'd say documenting is a good idea, too. And then check in the core (and WARN there) and remove the code from hardware drivers.

Quoted 6 lines

Thinking about this, I'd not require that a consumer disables a pwm before calling pwm_put. Eventually it might have good reasons to keep the PWM running (e.g. to keep a led in its state). So I'd restrict the documentation to the undisputed part that the hw driver should only touch the configuration in .apply (respectively .config, .set_polarity, .enable, .disable for legacy drivers).

Quoted 5 lines

What would you consider good reasons for keeping a PWM running when the consumer no longer needs it? I can't come up with any sane reason why we would want to allow that. It's normal to turn things off when you no longer need them. I do that with a computer, an oven and a shower, too. Why would a PWM be different?

I'm not sure I have a good use case. But I'm not certain enough that there is none and so I'd just say---keeping legacy drivers out of the discussion for simplicity:

A hardware driver should only modify the behaviour of the output pin in the .apply callback.

instead of:

The PWM consumer must disable the PWM before calling pwm_put. The backend driver can assume the PWM to be off when its .free callback is entered.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#6 lore raw
Quoted 46 lines · Uwe Kleine-König

Hello Thierry,

Quoted 23 lines · Thierry Reding
Quoted 14 lines · Uwe Kleine-König
Quoted 13 lines · Uwe Kleine-König
Quoted 11 lines · Thierry Reding
Quoted 7 lines · Uwe Kleine-König

while digging around in the pwm drivers I found drivers/pwm/pwm-lpc18xx-sct.c which in its .free callback calls pwm_disable() and pwm_set_duty_cycle(pwm, 0).

I think calling pwm_disable is wrong for two reasons:

a) pwm_disable is for PWM users, not for drivers

PWM controller drivers can still call pwm_disable(), though they should do so very carefully and not behind consumers' backs.

Quoted 7 lines
Quoted 3 lines

I'd say this is a bad idea because the driver knows in which callback this results and so there is complexity for no real benefit. When the core implemented some locking this could easily deadlock.

I think it would be beneficial to get rid of these calls. Would you agree to document calling pwm-API calls as forbidden as a first step and then work on fixing these?

Quoted 6 lines

I'm not a big fan of documenting anything as being forbidden. People can do whatever they want when nobody's checking and we don't have a way of enforcing anything either because they could just as well remove those enforcement measures. Remember, they have access to all the code, so the best we can do is discourage people from doing potentially stupid things by documenting expectations, requirements and limitations.

In my eyes the purpose of the PWM framework (and similar all other frameworks, too) is to abstract a common set of functions for a certain type of hardware. As such it is beneficial to all affected parties if the hardware drivers have a tight policy what is expected. This makes implementing stuff in the hardware driver easier, because people know what should be done and what not in a given callback. It makes pwm consumers' work easier, because they know what to expect from calling a certain API function and so increases the chances that a backlight driver that was only tested with say a rockchip PWM also works the same way when another machine makes use of the same backlight driver with an imx PWM.
This increases overall code quality.

Quoted 6 lines

Sure we cannot protect people from doing stupid things by patching stuff. But we can improve the situation for people who have to patch to actually fix broken stuff by documenting for them in the above scenario where the backlight driver doesn't work with the imx pwm if it's the backlight driver or the pwm driver that needs adaption to converge to a state where all pwm consumers work with all pwm backends.

I agree with the above. But I don't see how it is relevant to this particular discussion. I'm merely pointing out that we can't prevent people from using pwm_disable() in drivers if they choose to do so. Yes, we should avoid doing that in the upstream kernel if we consider it to be bad practice, and we can enforce it during review or reject patches that do so. If this is currently not clear, then by all means let's document it.

However, we have no means of controlling what people do when they work outside of upstream and we also have no way of prosecuting people, so "forbidding" something is pointless.

Quoted 26 lines
Quoted 16 lines
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines

So perhaps a good compromise would be to document that users are supposed to disable the PWM before they release it and then drivers (or the core) could go and check that they're really disabled when the chip is being removed and WARN() if not. That way we can flag all consumers that don't behave and fix them.

Yeah, I'd say documenting is a good idea, too. And then check in the core (and WARN there) and remove the code from hardware drivers.

Quoted 6 lines

Thinking about this, I'd not require that a consumer disables a pwm before calling pwm_put. Eventually it might have good reasons to keep the PWM running (e.g. to keep a led in its state). So I'd restrict the documentation to the undisputed part that the hw driver should only touch the configuration in .apply (respectively .config, .set_polarity, .enable, .disable for legacy drivers).

Quoted 5 lines

What would you consider good reasons for keeping a PWM running when the consumer no longer needs it? I can't come up with any sane reason why we would want to allow that. It's normal to turn things off when you no longer need them. I do that with a computer, an oven and a shower, too. Why would a PWM be different?

I'm not sure I have a good use case. But I'm not certain enough that there is none and so I'd just say---keeping legacy drivers out of the discussion for simplicity:

Quoted 8 lines

A hardware driver should only modify the behaviour of the output pin in the .apply callback.

instead of:

The PWM consumer must disable the PWM before calling pwm_put. The backend driver can assume the PWM to be off when its .free callback is entered.

I don't get it. On one hand you're arguing that we should put stricter rules into place so that consumers know what to expect and then you walk that back and say we shouldn't have stricter rules.

To reiterate my point: there's no reason to keep anything enabled after you're done using it. If you need to keep it enabled it means that you still use it. In terms of drivers this means that as long as a device is actively being used (backlight, fan, motor, ...), the consumer of the PWM should stay around. You don't unload a backlight driver if you want to keep using the backlight, right? The same is true for fans and any number of things. Once you no longer use the device the natural action is to turn it off. Consequently when you unload the PWM consumer driver the PWM should be disabled.

So I do think we should require consumers to disable a PWM before they release the PWM using pwm_put() and warn about it on pwmchip_remove().

Thierry

author
#7 lore raw

Hello Thierry,

Quoted 52 lines · Thierry Reding
Quoted 44 lines · Uwe Kleine-König
Quoted 23 lines · Thierry Reding
Quoted 14 lines · Uwe Kleine-König
Quoted 13 lines · Uwe Kleine-König
Quoted 11 lines · Thierry Reding
Quoted 7 lines · Uwe Kleine-König

while digging around in the pwm drivers I found drivers/pwm/pwm-lpc18xx-sct.c which in its .free callback calls pwm_disable() and pwm_set_duty_cycle(pwm, 0).

I think calling pwm_disable is wrong for two reasons:

a) pwm_disable is for PWM users, not for drivers

PWM controller drivers can still call pwm_disable(), though they should do so very carefully and not behind consumers' backs.

Quoted 7 lines
Quoted 3 lines

I'd say this is a bad idea because the driver knows in which callback this results and so there is complexity for no real benefit. When the core implemented some locking this could easily deadlock.

I think it would be beneficial to get rid of these calls. Would you agree to document calling pwm-API calls as forbidden as a first step and then work on fixing these?

Quoted 6 lines

I'm not a big fan of documenting anything as being forbidden. People can do whatever they want when nobody's checking and we don't have a way of enforcing anything either because they could just as well remove those enforcement measures. Remember, they have access to all the code, so the best we can do is discourage people from doing potentially stupid things by documenting expectations, requirements and limitations.

In my eyes the purpose of the PWM framework (and similar all other frameworks, too) is to abstract a common set of functions for a certain type of hardware. As such it is beneficial to all affected parties if the hardware drivers have a tight policy what is expected. This makes implementing stuff in the hardware driver easier, because people know what should be done and what not in a given callback. It makes pwm consumers' work easier, because they know what to expect from calling a certain API function and so increases the chances that a backlight driver that was only tested with say a rockchip PWM also works the same way when another machine makes use of the same backlight driver with an imx PWM.
This increases overall code quality.

Quoted 6 lines

Sure we cannot protect people from doing stupid things by patching stuff. But we can improve the situation for people who have to patch to actually fix broken stuff by documenting for them in the above scenario where the backlight driver doesn't work with the imx pwm if it's the backlight driver or the pwm driver that needs adaption to converge to a state where all pwm consumers work with all pwm backends.

Quoted 2 lines

I agree with the above. But I don't see how it is relevant to this particular discussion.

I also think this is irrelevant to the discussion. It was you who brought up this argument.

Quoted 6 lines

I'm merely pointing out that we can't prevent people from using pwm_disable() in drivers if they choose to do so. Yes, we should avoid doing that in the upstream kernel if we consider it to be bad practice, and we can enforce it during review or reject patches that do so. If this is currently not clear, then by all means let's document it.

Of course such a rule only applies to the vanilla kernel. And it is totally irrelevant if the rule states that something is forbidden or if it states that something must be done.

Quoted 35 lines

However, we have no means of controlling what people do when they work outside of upstream and we also have no way of prosecuting people, so "forbidding" something is pointless.

Quoted 31 lines
Quoted 16 lines
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines

So perhaps a good compromise would be to document that users are supposed to disable the PWM before they release it and then drivers (or the core) could go and check that they're really disabled when the chip is being removed and WARN() if not. That way we can flag all consumers that don't behave and fix them.

Yeah, I'd say documenting is a good idea, too. And then check in the core (and WARN there) and remove the code from hardware drivers.

Quoted 6 lines

Thinking about this, I'd not require that a consumer disables a pwm before calling pwm_put. Eventually it might have good reasons to keep the PWM running (e.g. to keep a led in its state). So I'd restrict the documentation to the undisputed part that the hw driver should only touch the configuration in .apply (respectively .config, .set_polarity, .enable, .disable for legacy drivers).

Quoted 5 lines

What would you consider good reasons for keeping a PWM running when the consumer no longer needs it? I can't come up with any sane reason why we would want to allow that. It's normal to turn things off when you no longer need them. I do that with a computer, an oven and a shower, too. Why would a PWM be different?

I'm not sure I have a good use case. But I'm not certain enough that there is none and so I'd just say---keeping legacy drivers out of the discussion for simplicity:

A hardware driver should only modify the behaviour of the output pin in the .apply callback.

instead of:

Quoted 7 lines
Quoted 3 lines

The PWM consumer must disable the PWM before calling pwm_put. The backend driver can assume the PWM to be off when its .free callback is entered.

I don't get it. On one hand you're arguing that we should put stricter rules into place so that consumers know what to expect and then you walk that back and say we shouldn't have stricter rules.

I think the rules for hardware drivers should be strict. And I think that the PWM framework should be as cooperative as possible to its consumers. That's according to the motto "Be conservative in what you do, be liberal in what you accept from others."

Quoted 10 lines

To reiterate my point: there's no reason to keep anything enabled after you're done using it. If you need to keep it enabled it means that you still use it. In terms of drivers this means that as long as a device is actively being used (backlight, fan, motor, ...), the consumer of the PWM should stay around. You don't unload a backlight driver if you want to keep using the backlight, right? The same is true for fans and any number of things. Once you no longer use the device the natural action is to turn it off. Consequently when you unload the PWM consumer driver the PWM should be disabled.

Quoted 2 lines

So I do think we should require consumers to disable a PWM before they release the PWM using pwm_put() and warn about it on pwmchip_remove().

I wouldn't enforce this, but I won't argue here.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
author
#8 lore raw

PATCH 1/1 pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

In reply to Thierry Reding

Regarding the .request case: The consumer might be interested in taking over the configured state from the boot loader. So the initially configured state should be retained.

For the free case the PWM consumer is responsible to disable the PWM before calling pwm_release and there are three subcases to consider:

a) The pwm is already off. Then there is no gain in disabling the PWM once more.
b) The pwm is still running and there is a good reason for that. (Not sure this is a valid case, I cannot imagine such a good reason.) Then it is contra productive to disable the pwm.
c) The pwm is still running because the consumer failed to disable the PWM. Then the consumer needs fixing and there is little incentive to paper over the problem in the backend driver.

This aligns the lpc18xx-sct driver to the other PWM drivers that also don't reconfigure the hardware in .request and .free.

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/pwm/pwm-lpc18xx-sct.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/pwm/pwm-lpc18xx-sct.c +0 -3
index d7f5f7de030d..475918d9f543 100644
@@ -296,7 +296,6 @@ static int lpc18xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	set_bit(event, &lpc18xx_pwm->event_map);
 	lpc18xx_data->duty_event = event;
-	lpc18xx_pwm_config_duty(chip, pwm, pwm_get_duty_cycle(pwm));
 
 	return 0;
 }

@@ -306,8 +305,6 @@ static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
 	struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
 
-	pwm_disable(pwm);
-	pwm_set_duty_cycle(pwm, 0);
 	clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map);
 }
Signature · 1 line
2.19.1
author
#9 lore raw

PATCH 1/1 pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

Hello,

FTR: The e-mail address of Joachim Eastwood doesn't work. I got a bounce back saying "The email account that you tried to reach is over quota. Please direct the recipient to
https://support.google.com/mail/?p=OverQuotaPerm."

If you remember this when replying to this thread I think the right thing is to drop him from the recipents.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
maintainer
#10 lore raw

PATCH 1/1 pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

In reply to Uwe Kleine-König

Hello Uwe,

Quoted 7 lines · Uwe Kleine-K?nig

Regarding the .request case: The consumer might be interested in taking over the configured state from the boot loader. So the initially configured state should be retained.

For the free case the PWM consumer is responsible to disable the PWM before calling pwm_release and there are three subcases to consider:

the changes are fine per se, but please split them into two.

Probably pwm_disable() misusage began spreading from commit 54b2a999a1675.

--
Best wishes,
Vladimir

author
#11 lore raw

PATCH 1/1 pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

Quoted 7 lines · Vladimir Zapolskiy

Hello Uwe,

Quoted 4 lines · Uwe Kleine-K?nig

Regarding the .request case: The consumer might be interested in taking over the configured state from the boot loader. So the initially configured state should be retained.

Quoted 7 lines
Quoted 3 lines

For the free case the PWM consumer is responsible to disable the PWM before calling pwm_release and there are three subcases to consider:

the changes are fine per se, but please split them into two.

Probably pwm_disable() misusage began spreading from commit 54b2a999a1675.

I see little benefit, but if that's the only problem I can split.

Note that the behaviours of .request and .free are not unrelated. Currently because .free sets a duty cycle of 0 we have pwm_get_duty_cycle always return 0 in .request.

Waiting on what Thierry thinks.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#12 lore raw

PATCH 1/1 pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

In reply to Vladimir Zapolskiy
Quoted 7 lines · Vladimir Zapolskiy

Hello Uwe,

Quoted 4 lines · Uwe Kleine-K?nig

Regarding the .request case: The consumer might be interested in taking over the configured state from the boot loader. So the initially configured state should be retained.

Quoted 7 lines
Quoted 3 lines

For the free case the PWM consumer is responsible to disable the PWM before calling pwm_release and there are three subcases to consider:

the changes are fine per se, but please split them into two.

Probably pwm_disable() misusage began spreading from commit 54b2a999a1675.

It's not really misusage to call pwm_disable(). It's basically just a shortcut for ->disable() or the atomic equivalent for it. So I think the commit that you point to is doing exactly the right thing. Also note that that commit was made 6 years ago, and a lot of things have changed since then.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181116/94ce7e09/attachment.sig>

author
#13 lore raw

PATCH 1/1 pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

Hello Thierry,

Quoted 10 lines · Thierry Reding
Quoted 9 lines · Vladimir Zapolskiy
Quoted 7 lines · Uwe Kleine-K?nig

Regarding the .request case: The consumer might be interested in taking over the configured state from the boot loader. So the initially configured state should be retained.

For the free case the PWM consumer is responsible to disable the PWM before calling pwm_release and there are three subcases to consider:

Quoted 6 lines
Quoted 3 lines

the changes are fine per se, but please split them into two.

Probably pwm_disable() misusage began spreading from commit 54b2a999a1675.

It's not really misusage to call pwm_disable(). It's basically just a shortcut for ->disable() or the atomic equivalent for it.

I'd say that pwm_disable() is not a shortcut but the long way round to .disable() at best.

And that calling pwm_disable() from the low level driver doesn't result in problems is just luck because the pwm framework is such a thin wrapper. If it would do locking, .free might already hold the lock and pwm_disable might try to grab it again. If it would do more checking that only a caller of pwm_get calls pwm_disable, the check would trigger because the consumer already called pwm_put and the lowlevel driver didn't use pwm_get.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Applied
#14 lore raw

PATCH 1/1 pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

In reply to Uwe Kleine-König
Quoted 16 lines · Uwe Kleine-K?nig

Regarding the .request case: The consumer might be interested in taking over the configured state from the boot loader. So the initially configured state should be retained.

For the free case the PWM consumer is responsible to disable the PWM before calling pwm_release and there are three subcases to consider:

a) The pwm is already off. Then there is no gain in disabling the PWM once more.
b) The pwm is still running and there is a good reason for that. (Not sure this is a valid case, I cannot imagine such a good reason.) Then it is contra productive to disable the pwm.
c) The pwm is still running because the consumer failed to disable the PWM. Then the consumer needs fixing and there is little incentive to paper over the problem in the backend driver.

Quoted 7 lines

This aligns the lpc18xx-sct driver to the other PWM drivers that also don't reconfigure the hardware in .request and .free.

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/pwm/pwm-lpc18xx-sct.c | 3 ---
 1 file changed, 3 deletions(-)

Applied, with some minor fixes to the commit message (pwm -> PWM, pwm_release -> pwm_put(), ...).

Thanks,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181116/3730d634/attachment.sig>

author
#15 lore raw

PATCH 1/1 pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

Hello Thierry,

Quoted 2 lines · Thierry Reding

Applied, with some minor fixes to the commit message (pwm -> PWM, pwm_release -> pwm_put(), ...).

Thanks. I wonder though why the commit didn't make it into next yet. Your for-next branch doesn't contain anything that's not already in Linus Torvalds' tree. I assume this isn't on purpose and you just didn't push your changes out?

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#16 lore raw

PATCH 1/1 pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

Quoted 10 lines · Uwe Kleine-K?nig

Hello Thierry,

Quoted 2 lines · Thierry Reding

Applied, with some minor fixes to the commit message (pwm -> PWM, pwm_release -> pwm_put(), ...).

Thanks. I wonder though why the commit didn't make it into next yet. Your for-next branch doesn't contain anything that's not already in Linus Torvalds' tree. I assume this isn't on purpose and you just didn't push your changes out?

Pushed now.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181120/68f35216/attachment.sig>