Message-ID: <20201207214641.64xom6njb3mlnhgp@pengutronix.de>
Date:   Mon, 7 Dec 2020 22:46:41 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Sean Young <sean@...s.org>,
        Lino Sanfilippo <LinoSanfilippo@....de>, lee.jones@...aro.org,
        nsaenzjulienne@...e.de, f.fainelli@...il.com, rjui@...adcom.com,
        sbranden@...adcom.com, bcm-kernel-feedback-list@...adcom.com,
        linux-pwm@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic
 configuration

Hello Thierry,

On Mon, Dec 07, 2020 at 04:29:36PM +0100, Thierry Reding wrote:
> On Mon, Dec 07, 2020 at 02:52:09PM +0100, Uwe Kleine-König wrote:
> > 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.

You describe exactly the function I had in mind when talking about
pwm_round_state. In my eyes pwm_round_state is the better name, because
it makes it obvious that it modifies the passed pwm_state. The clk
framework also has clk_round_rate with similar semantics.

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

Yeah, I think it will not be very useful to be used directly by
consumers in most cases. The driver's callback can however be used in
helper functions provided by the framework. The pwm-ir-tx driver would
then do:

	struct pwm_state state = {
		.period = carrier_period,
		.duty_cycle = 0,
		...
	};

	ret = pwm_round_nearest(mypwn, &state);
	if (!ret)
		... error handling

and then inspect state to judge if it is good enough and use that.

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

I think we can agree that with consumers having different needs we
should be able to give all of them what they need. Preferably in a way
that lowlevel drivers must do only something simple and the main
complexity lives in common framework code.

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

And you want to adapt all drivers (maybe on a on-demand base) to
implement "round-down", "round-period-to-nearest-but-duty-down" and all
other demands that my come up in the future? Did you notice how
difficult "round-nearest" is even in the simple example with a single
divider in my mail to Sean earlier today? I don't want this in several
drivers. And note this isn't even workable, consider the servo motor
example where a 1ms pulse means move forward and 2ms pulse means move
backwards. In this case you really want to know before applying the
state that the resulting pulses will be longer than (say) 1.8 ms or
shorter than 1.2 ms. And note that adding a "rounding" member to state
doesn't prevent us touching all drivers. If I request a certain state
with round-nearest and the driver is not aware of rounding it might use
round-down and even applies this. Also the PWM driver should not be free
to say "Ohh, the consumer requested 2ms and rounding up, but 1ms is the
best I can do, so that's what they get". So I might drive my vehicle
into a house and won't even notice before something bad happens.

This convinces me that it's impossible to provide the needed features to
consumers without adding a new callback that queries the HW capabilities
without modifying the output.

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

I cannot follow you. Why do you want to reject nearest requests? There
should always be a single state that is nearest to a given request. If
you request a period length of 2 years the actually returned state might
use a considerably shorter period, but there should be no need to reject
this. (It might only be good if this can be noticed before the state
with the shorter period is put into action.)

For round-down some states are impossible, but round-nearest should be
fine.

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

That's why I think all drivers should just implement "round down" and
framework logic can implement the necessary logic to still provide
consumers a "round nearest" or any other necessary rounding strategy.

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

I don't agree. If I want my motor to move forward, I don't want the PWM
driver in use be lax enough to result in a backwards move.

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

My thought to go forward is:

- Introduce a new callback "round_state" or "round_down_state" to make
  it more obvious which behaviour is expected.
- For all drivers implementing this callback, enforce that
  .apply(.round_down_state(somestate)) behaves identically to
  .apply(somestate) and that .round_down_state indeed rounds down.
- Implement a pwm_round_nearest_state() function that only works for
  lowlevel drivers implementing .round_down_state. This way it can rely
  on the above item and so can be implemented without too much hassle.

Also all drivers can just stay as they are and as soon as someone
implements the round_down_state callback the driver becomes more useful.
Until this is the case they just continue to work as they do today, too.

Best regards
Uwe

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists