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.
> > So I don't see that "warning" would really help for this particular case.
> >
> > > But before doing that I think it would be sensible to also fix the rules
> > > how the round_state callback is supposed to round.
> >
> > I'm not quite sure I grok exactly what you're planning, but I would
> > much appreciate if you didn't break things on the way toward fixing
> > them ;)
>
> There are currently no rules how the driver should behave for example if
> the consumer requests
>
> .duty_cycle = 10, .period = 50
>
> and the hardware can only implement multiples of 3 for both values. The
> obvious candidates are:
>
> - .duty_cycle = 9, .period = 51 (round nearest for both)
> - .duty_cycle = 12, .period = 51 (round up)
> - .duty_cycle = 9, .period = 48 (round down)
> - .duty_cycle = 9, .period = 45 (round duty_cycle and keep proportion)
> - return error (which code?)
>
> And there are some other variants (e.g. round duty_cycle to nearest and
> period in the same direction) that might be sensible.
I will note that I had to deal with some of this recently when I
wanted to try to replicate the exact voltage levels for "vdd_log" from
downstream in rk3288-veyron devices. See commit 864c2fee4ee9 ("ARM:
dts: rockchip: Add vdd_logic to rk3288-veyron") in Heiko's tree (or
just look in linux-next)
> Also it should be possible to know the result before actually
> configuring the hardware. Otherwise things might already go wrong
> because the driver implements a setting that is too far from the
> requested configuration.
Later in this thread Thierry didn't like the "round rate" idea due to
races. One way to solve that could be to indicate to the PWM
framework which direction you'd like it to error in: a higher duty
cycle or a lower one.
-Doug
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-rockchip
