[PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Thierry Reding]

On Mon, Dec 07, 2020 at 02:52:09PM +0100, Uwe Kleine-König wrote:
> Hello Sean,
> 
> On Mon, Dec 07, 2020 at 09:43:20AM +0000, Sean Young wrote:
> > Thank you for taking the time to explain your thinking.
> 
> I'm happy you have an open ear for it. With this I really enjoy spending
> the time to find the right arguments and examples.
> 
> > On Mon, Dec 07, 2020 at 09:16:28AM +0100, Uwe Kleine-König wrote:
> > > On Sun, Dec 06, 2020 at 02:19:41PM +0000, Sean Young wrote:
> > > > On Sat, Dec 05, 2020 at 08:25:10PM +0100, Uwe Kleine-König wrote:
> > > > > On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote:
> > > > > > What real life uses-cases are there for round down? If you want to round
> > > > > > down, is there any need for round up?
> > > > > 
> > > > > The scenario I have in mind is for driving a motor. I have to admit
> > > > > however that usually the period doesn't matter much and it's the
> > > > > duty_cycle that defines the motor's speed. So for this case the
> > > > > conservative behaviour is round-down to not make the motor run faster
> > > > > than expected.
> > > > 
> > > > I am reading here that for driving motors, only the duty cycle matters,
> > > > not the period.
> > > 
> > > There is an upper limit (usually around 1 ms) for the period, but if you
> > > choose 0.1 ms or 0.001 ms doesn't matter much AFAICT.
> > > 
> > > @Thierry: Do you have further use cases in mind?
> 
> I asked in the hardware department of the company I work for and they
> had another usecase: Motors where for example a 1 ms pulse means "move
> forwards" and 2 ms means "move backwards". They had the same idea as I
> had: You want to know beforehand the result of a given
> pwm_apply_state().

I've occasionally considered the idea of adding a pwm_check_state() API
that would allow you to pass in a struct pwm_state and get a result as
to whether it can be applied or not. It's never really made much sense
because pwm_apply_state() can already return failure if it can't apply
the state.

However, if we need some way for consumers to be more clever about state
changes, then something like pwm_check_state() might be more useful if,
in addition to just checking the validity/applicability of the state we
can also return the state that would be applied after all the hardware-
specific rounding.

That way the consumer can use it to perform some checks on the resulting
state and submit it if satisfied with the outcome. Alternatively, if it
determines that the state is not viable, it can retry with different
values.

I'm not sure how useful that really is because it makes the usage really
difficult on the consumer side. Perhaps there's no need for this anymore
if the consumer is able to specify the rounding, so perhaps we should
concentrate on that API first.

> > > > > For other usecases (fan, backlight, LED) exactness typically doesn't
> > > > > matter that much.
> > > > 
> > > > So, the use-cases you have are driving motor, fan, backlight, and led.
> > > > And in all these cases the exact Hz does not matter.
> > > > 
> > > > The only uses case where the exact Hz does matter is pwm-ir-tx. 
> > > > 
> > > > So, I gather there are no use-cases for round-down. Yes, should round-down
> > > > be needed, then this is more difficult to implement if the driver always
> > > > does a round-closest. But, since there is no reason to have round-down,
> > > > this is all academic.
> > > > 
> > > > Your policy of forcing new pwm drivers to use round-down is breaking
> > > > pwm-ir-tx.
> > > 
> > > So you're indeed suggesting that the "right" rounding strategy for
> > > lowlevel drivers should be:
> > > 
> > >  - Use the period length closest to the requested period (in doubt round
> > >    down?)
> > >  - With the chosen period length use the biggest duty_cycle not bigger
> > >    than the requested duty_cycle.
> > > 
> > > While this seems technically fine I think for maintenance this is a
> > > nightmare.
> > > 
> > > My preference would be to stick to the rounding strategy we used so far
> > > (i.e.:
> > > 
> > >  - Use the biggest period length not bigger than the requested period
> > >  - With the chosen period length use the biggest duty_cycle not bigger
> > >    than the requested duty_cycle.
> > > 
> > > ) and for pwm-ir-tx add support to the PWM API to still make it possible
> > > (and easy) to select the best setting.
> > > 
> > > The reasons why I think that this rounding-down strategy is the best
> > > are (in order of importance):
> > > 
> > >  - It is easier to implement correctly [1]
> > 
> > Yes, you are right. You have given a great example where a simple
> > DIV_ROUND_CLOSEST() does not give the result you want.
> > 
> > >  - Same rounding method for period and duty cycle
> > >  - most drivers already do this (I think)
> > > 
> > > The (IMHO nice) result would then mean:
> > > 
> > >  - All consumers can get the setting they want; and
> > 
> > Once there is a nice pwm api for selecting round-nearest, then yes.
> > 
> > For the uses cases you've given, fan, backlight, and led a round-nearest
> > is the rounding they would want, I would expect.
> 
> maybe, yes. Maybe it is also not important enough to spend the extra
> cycles getting round nearest and so sticking to round-down is good
> enough.

Yeah, I think in most cases currently the consumer just doesn't care
enough and things happen to just work. Maybe they're not perfect, but
good enough.

One of the reasons I was reluctant to introduce a "default" rounding
behaviour is precisely because it's not clear cut, so in some cases the
default may not be what we really want, such as in the pwm-ir-tx case
here.

> > >  - Code in lowlevel drivers is simple and the complexity is in common
> > >    code and so a single place.
> > > 
> > > And it would also allow the pwm-ir-tx driver to notice if the PWM to be
> > > used can for example only support frequencies under 400 kHz.
> > 
> > I doubt pwm-ir-tx cares about this, however it is a nice-to-have. It would
> > also be nice if the rounding could be used with atomic configuration
> > as well.
> 
> I cannot follow, you created 11fc4edc483bea8bf0efa0cc726886d2342f6fa6
> because 476.2 Mhz was too bad. So you seem to be interested in
> deviations and part of the problem is that you don't get feedback about
> how your request is fulfilled.
> 
> > Please let me know when/if this new API exists for pwm so that pwm-ir-tx
> > can select the right rounding.
> 
> Given that the bcm2835 driver is quite trivial I would be happy to
> create a series that "fixes" the driver to round down and provide a
> prototype for pwm_round_nearest for you to test on pwm-ir-tx. A willing
> tester and a real use-case were the single two things that stopped me
> investing time here.

I'd like to avoid adding a new function for this functionality and
instead add a rounding type field to the PWM state. Also, in doing so we
should be able to keep the status quo for everyone by making the default
rounding behaviour "don't care", which is what basically everyone right
now uses. In specific cases like pwm-ir-tx we can adjust the rounding to
become "nearest".

That said, the rounding behaviour is not something that the API can
guarantee, because if we start rejecting "nearest" requests, we might
end up breaking a bunch of setups that want "nearest" but where the
controller doesn't support it. At the same time I don't want to make it
a prerequisite that all drivers implement all possible rounding
behaviours because it puts a very high burden on the driver writer that
may not need (or have a way of testing) anything other than "nearest",
or "round down", or whatever.

So I think from an API perspective the rounding behaviour would always
have to be a sort of "hint" to the driver to specify what the consumer
wants to use, but it should never fail to apply a state purely on this
rounding behaviour, so returning some state that's the best the driver
can do is better than failing if it doesn't know some mode.

This also ensures that existing drivers will be able to continue to work
the same way they always have, and the new mechanism is merely something
to improve the use-cases where we need more precise control.

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/20201207/f360c269/attachment.sig>