On Thu, May 02, 2019 at 10:42:43AM +0200, Uwe Kleine-König wrote:
> Hello Boris,
>
> On Thu, May 02, 2019 at 10:09:51AM +0200, Boris Brezillon wrote:
> > On Thu, 2 May 2019 09:33:26 +0200
> > Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> >
> > > Hello Boris,
> > >
> > > On Thu, May 02, 2019 at 09:16:38AM +0200, Boris Brezillon wrote:
> > > > On Mon, 29 Apr 2019 11:04:20 -0700
> > > > Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Sun, Apr 28, 2019 at 11:56 PM Uwe Kleine-König
> > > > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > I'm not sure if I'm misreading you, but I thought I'd add here before
> > > > > > > this expires out of my inbox:
> > > > > > >
> > > > > > > On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> > > > > > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > > > > > > > My intention here is more to make all drivers behave the same way and
> > > > > > > > because only two drivers updated the pwm_state this was the variant I
> > > > > > > > removed.
> > > > > > >
> > > > > > > To be clear, this patch on its own is probably breaking things. Just
> > > > > > > because the other drivers don't implement the documented behavior
> > > > > > > doesn't mean you should break this driver. Maybe the others just
> > > > > > > aren't used in precise enough scenarios where this matters.
> > > > > > >
> > > > > > > > When you say that the caller might actually care about the exact
> > > > > > > > parameters I fully agree. In this case however the consumer should be
> > > > > > > > able to know the result before actually applying it. So if you do
> > > > > > > >
> > > > > > > > pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > > > > > > >
> > > > > > > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > > > > > > bad things you want to know about already happend. So my idea is a new
> > > > > > > > function pwm_round_state() that does the adaptions to pwm_state without
> > > > > > > > applying it to the hardware. After that pwm_apply_state could do the
> > > > > > > > following:
> > > > > > > >
> > > > > > > > rstate = pwm_round_state(pwm, state)
> > > > > > > > pwm.apply(pwm, state)
> > > > > > > > gstate = pwm_get_state(pwm)
> > > > > > > >
> > > > > > > > if rstate != gstate:
> > > > > > > > warn about problems
> > > > > > >
> > > > > > > For our case (we're using this with pwm-regulator), I don't recall [*]
> > > > > > > we need to be 100% precise about the period, but we do need to be as
> > > > > > > precise as possible with the duty:period ratio -- so once we get the
> > > > > > > "feedback" from the underlying PWM driver what the real period will
> > > > > > > be, we adjust the duty appropriately.
> > > > > >
> > > > > > I admit that I didn't understood the whole situation and (some) things
> > > > > > are worse with my patches applied. I still think that changing the
> > > > > > caller's state variable is bad design, but of course pwm_get_state
> > > > > > should return the currently implemented configuration.
> > > > >
> > > > > Regardless of the pros and cons of the current situation, hopefully
> > > > > we're in agreement that we shouldn't break existing users? In general
> > > > > I'll probably stay out of the debate as long as we end somewhere that
> > > > > pwm_regulator is able to somehow know the actual state that it
> > > > > programmed into the hardware.
> > > > >
> > > > > +Boris too in case he has any comments.
> > > >
> > > > Well, the pwm_round_state() approach sounds okay to me, though I don't
> > > > really see why it's wrong to adjust the state in pwm_apply_state()
> > > > (just like clk_set_rate() will round the rate for you by internally
> > > > calling clk_round_rate() before applying the config).
> > >
> > > This are two orthogonal things. The "should pwm_apply_state change the
> > > state argument" isn't really comparable to the clk stuff, as there only
> > > the frequency is provided that is passed by value, not by reference as
> > > the PWM state.
> > >
> > > The key argument for me to *not* change it is that it might yield
> > > surprises, still more as today most drivers don't adapt. An -- I admit
> > > constructed, not real-word -- case where adaption would go wrong is:
> > >
> > > pwm_apply_state(pwm1, &mystate);
> > > pwm_apply_state(pwm2, &mystate);
> >
> > I see, but it's also clearly documented that pwm_apply_state() might
> > adjust the period/duty params [1], and it's not like we have a lot of
> > PWM users converted to use pwm_apply_state(), so I'd expect them to be
> > aware of that and use a reference pwm_state if they need to apply it
> > to different devices.
>
> If we accept that pwm_apply_state should adapt its state argument that
> would be ok for me, too. Then however we should make this consistent and
> consider a deviation that is not reported there as a bug.
>
> Note there are also more subtile problems. For example something like:
>
> def enable(self):
> state = pwm_get_state(self.pwm)
> state.duty_cycle *= 2
> pwm_apply_state(self.pwm, state)
>
> def disable(self):
> state = pwm_get_state(self.pwm)
> state.duty_cycle /= 2
> pwm_apply_state(self.pwm, state)
>
> doesn't guarantee that the sequence enable(); disable(); is idempotent.
> So my favourite would be to not modfies the caller's copy of state for
> pwm_apply_state(). (Note, this doesn't necessarily have implications
> about the semantik of the lowlevel driver callbacks.) Still
> pwm_get_state() should work and yield the corrected settings.
>
> > > > Note that pwm_config() is doing exactly the same: it adjusts the
> > > > config to HW caps, excepts in that case you don't know it.
> > >
> > > I don't see what you mean here. I don't see any adaption.
> >
> > I mean that the config you end up is not necessarily what you asked
> > for, and pwm_apply_state() was making that explicit by returning the
> > actual PWM state instead of letting the user think its config has been
> > applied with no adjustment.
>
> Ah. Of course the lowlevel driver has to work with the capabilities of
> the hardware. That is something we cannot get rid of. It's just a
> question how we communicate this to the consumer.
>
> > > > I do understand that some users might want to check how the HW will
> > > > adjust the period/duty before applying the new setup, and in that
> > > > regard, having pwm_round_rate() is a good thing. But in any case, it's
> > > > hard for the user to decide how to adjust things to get what it wants
> > > > (should he increase/decrease the period/duty?).
> > >
> > > It depends on the use case. For one of them I suggested an algorithm.
> >
> > Yes, I was just trying to say that passing a PWM state to
> > pwm_round_state() is not enough, we need extra info if we want to make
> > it useful, like the rounding policy, the accepted deviation on period,
> > duty or the duty/period ratio, ....
>
> Ack. My suggestion is that round_rate should do:
>
> if polarity is unsupported:
> polarity = !polarity
> duty_cycle = period - duty_cycle
This should really be up to the consumer. The PWM framework or driver
doesn't know whether or not the consumer cares about the polarity or
whether it only cares about the power output.
> period = biggest supportable period <= requested period, 0 if no
> such period exists.
>
> duty_cycle = biggest supportable duty cycle <= requested
> duty_cycle, 0 if no such value exists
This doesn't really work. We need some way to detect "value does not
exist" that is different from value == 0, because value == 0 is a
legitimate use-case.
> This would allow to let the consumer (or framework helper function)
> decide which deviation is ok.
So what's the consumer supposed to do if it gets back these values? How
does it know how to scale them if the deviation is not okay? What in
case the hardware can do achieve a good period that is slightly bigger
than the requested period and which would give a very good result?
Thierry
> > > > My impression is that most users care about the duty/period ratio with
> > > > little interest in accurate period settings (as long as it's close
> > > > enough to what they expect of course). For the round-up/down/closest
> > > > aspect, I guess that's also something we can pass to the new API. So,
> > > > rather than passing it a duty in ns, maybe we could pass it a ratio
> > > > (percent is probably not precise enough for some use cases, so we could
> > > > make it per-million).
> > >
> > > Yeah, something like that would be good. Still for the device drivers I
> > > would use the callback I suggested because that is easier to implement.
> >
> > Sorry, I just joined the discussion and couldn't find the email where
> > you suggested a new driver hook to deal with that.
>
> https://www.spinics.net/lists/linux-pwm/msg09627.html
>
> > > This way the complexity is once in the framework instead of in each
> > > driver.
> >
> > I think we want to make it possible for drivers to do complex
> > adjustments, and that implies passing all info to the new driver
> > hook. The core can then provide generic helpers for simple use-cases
> > (approximation for static period/duty steps, where no reclocking is
> > involved).
>
> The problem is that it is hard to come up with a formalism to map "all
> info" because there are so many different ways to prefer one
> configuration over another. I believe we won't be able to design a sane
> callback prototype that allows to map all use cases.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Linux-rockchip mailing list Linux-rockchip@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/linux-rockchip
