is pwm_put supposed to stop a PWM?

is pwm_put supposed to stop a PWM?

[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
 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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: is pwm_put supposed to stop a PWM?

[Thierry Reding]

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

On Thu, Oct 25, 2018 at 09:45:24PM +0200, Uwe Kleine-König wrote:
> 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.

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

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: is pwm_put supposed to stop a PWM?

[Uwe Kleine-König]

Hello Thierry,

On Mon, Oct 29, 2018 at 12:48:35PM +0100, Thierry Reding wrote:
> On Thu, Oct 25, 2018 at 09:45:24PM +0200, Uwe Kleine-König wrote:
> > 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.

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

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

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

OK, will prepare a patch when I come around.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: is pwm_put supposed to stop a PWM?

[Uwe Kleine-König]

Hello Thierry,

On Sat, Nov 03, 2018 at 03:49:20PM +0100, Uwe Kleine-König wrote:
> On Mon, Oct 29, 2018 at 12:48:35PM +0100, Thierry Reding wrote:
> > On Thu, Oct 25, 2018 at 09:45:24PM +0200, Uwe Kleine-König wrote:
> > > 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.

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?

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: is pwm_put supposed to stop a PWM?

[Thierry Reding]

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

On Wed, Nov 14, 2018 at 10:30:07AM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Sat, Nov 03, 2018 at 03:49:20PM +0100, Uwe Kleine-König wrote:
> > On Mon, Oct 29, 2018 at 12:48:35PM +0100, Thierry Reding wrote:
> > > On Thu, Oct 25, 2018 at 09:45:24PM +0200, Uwe Kleine-König wrote:
> > > > 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.
> 
> 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.

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

Yeah, that's fair.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: is pwm_put supposed to stop a PWM?

[Uwe Kleine-König]

Hello Thierry,

On Wed, Nov 14, 2018 at 12:50:25PM +0100, Thierry Reding wrote:
> On Wed, Nov 14, 2018 at 10:30:07AM +0100, Uwe Kleine-König wrote:
> > On Sat, Nov 03, 2018 at 03:49:20PM +0100, Uwe Kleine-König wrote:
> > > On Mon, Oct 29, 2018 at 12:48:35PM +0100, Thierry Reding wrote:
> > > > On Thu, Oct 25, 2018 at 09:45:24PM +0200, Uwe Kleine-König wrote:
> > > > > 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.
> > 
> > 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.

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.

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: is pwm_put supposed to stop a PWM?

[Thierry Reding]

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

On Thu, Nov 15, 2018 at 09:42:31AM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Nov 14, 2018 at 12:50:25PM +0100, Thierry Reding wrote:
> > On Wed, Nov 14, 2018 at 10:30:07AM +0100, Uwe Kleine-König wrote:
> > > On Sat, Nov 03, 2018 at 03:49:20PM +0100, Uwe Kleine-König wrote:
> > > > On Mon, Oct 29, 2018 at 12:48:35PM +0100, Thierry Reding wrote:
> > > > > On Thu, Oct 25, 2018 at 09:45:24PM +0200, Uwe Kleine-König wrote:
> > > > > > 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.
> > > 
> > > 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.
> 
> 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.

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.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: is pwm_put supposed to stop a PWM?

[Uwe Kleine-König]

Hello Thierry,

On Thu, Nov 15, 2018 at 04:43:45PM +0100, Thierry Reding wrote:
> On Thu, Nov 15, 2018 at 09:42:31AM +0100, Uwe Kleine-König wrote:
> > On Wed, Nov 14, 2018 at 12:50:25PM +0100, Thierry Reding wrote:
> > > On Wed, Nov 14, 2018 at 10:30:07AM +0100, Uwe Kleine-König wrote:
> > > > On Sat, Nov 03, 2018 at 03:49:20PM +0100, Uwe Kleine-König wrote:
> > > > > On Mon, Oct 29, 2018 at 12:48:35PM +0100, Thierry Reding wrote:
> > > > > > On Thu, Oct 25, 2018 at 09:45:24PM +0200, Uwe Kleine-König wrote:
> > > > > > > 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.
> > > > 
> > > > 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.
> > 
> > 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.
> 
> 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.

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

> 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.
> 
> > > > > > 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).
> > > 
> > > 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.
> 
> 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."

> 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().

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

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

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 <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpc18xx-sct.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
index d7f5f7de030d..475918d9f543 100644
--- a/drivers/pwm/pwm-lpc18xx-sct.c
+++ b/drivers/pwm/pwm-lpc18xx-sct.c
@@ -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);
 }
 
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

[Vladimir Zapolskiy]

Hello Uwe,

On 11/16/2018 08:52 AM, Uwe Kleine-König wrote:
> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

[Uwe Kleine-König]

On Fri, Nov 16, 2018 at 11:22:49AM +0200, Vladimir Zapolskiy wrote:
> Hello Uwe,
> 
> On 11/16/2018 08:52 AM, Uwe Kleine-König wrote:
> > 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.

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 952 bytes --]

On Fri, Nov 16, 2018 at 11:22:49AM +0200, Vladimir Zapolskiy wrote:
> Hello Uwe,
> 
> On 11/16/2018 08:52 AM, Uwe Kleine-König wrote:
> > 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.

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1314 bytes --]

On Fri, Nov 16, 2018 at 07:52:08AM +0100, Uwe Kleine-König wrote:
> 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 <u.kleine-koenig@pengutronix.de>
> ---
>  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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

[Uwe Kleine-König]

Hello Thierry,

On Fri, Nov 16, 2018 at 11:01:14AM +0100, Thierry Reding wrote:
> On Fri, Nov 16, 2018 at 11:22:49AM +0200, Vladimir Zapolskiy wrote:
> > On 11/16/2018 08:52 AM, Uwe Kleine-König wrote:
> > > 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.
> 
> 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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

[Uwe Kleine-König]

Hello Thierry,

On Fri, Nov 16, 2018 at 11:05:00AM +0100, Thierry Reding wrote:
> 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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] pwm: lpc18xx-sct: don't reconfigure PWM in .request and .free

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 540 bytes --]

On Mon, Nov 19, 2018 at 08:55:28PM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Fri, Nov 16, 2018 at 11:05:00AM +0100, Thierry Reding wrote:
> > 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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel