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

[Boris Brezillon <boris.brezillon-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>]

On Mon, 29 Apr 2019 11:04:20 -0700
Doug Anderson <dianders@chromium.org> wrote:

> Hi,
> 
> On Sun, Apr 28, 2019 at 11:56 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> 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@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.  
> >
> > 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). 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 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?).

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip