Re: [PATCH v2 1/3] pwm: rockchip: Don't update the state for the caller of pwm_apply_state()

Brian Norris
2019-04-19T00:27:05Z

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@pengutronix.de> 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.

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

Thanks,
Brian

[*] Working off a somewhat stale memory and a refresher pass over the code.

> Apart from that I agree that pwm_get_state should return the actually
> implemented configuration.
>
> Best regards
> Uwe

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org