Re: [PATCH v3 00/12] pwm: add support for atomic update

[Thierry Reding]

On Thu, Feb 04, 2016 at 03:01:50PM +0100, Boris Brezillon wrote:
> Hi Mark, Thierry,
> 
> On Thu, 4 Feb 2016 11:02:03 +0000
> Mark Brown <broonie@xxxxxxxxxx> wrote:
> 
> > On Wed, Feb 03, 2016 at 11:04:20AM -0800, Doug Anderson wrote:
> > 
> > > Sure.  ...but you agree that somehow you need a new API call for this,
> > > right?  Somehow the PWM regulator needs to be able to say that it
> > > wants the hardware state, not the initial state as specified in the
> > > device tree.
> > 
> > Wouldn't the most direct way to do that be to just not specify anything
> > in the DT?  If there *is* something in DT but we ignore it that's a bit
> > weird.
> 
> Just adding some inputs on this specific aspect. The reason we have to
> specify a period (and, to a lesser extent, the polarity) in the DT or
> PWM lookup table is because what most PWM users want is to specify a
> dutycycle relatively to a predefined period value.

That's not quite correct. The reason why we need the information in DT
is because it can't be derived from anything else. It is board-specific
data for which there's no heuristic that will work in all cases.

> If we decide to remove those information from the DT, then you'll need
> a way to define it somewhere else, and then the is question is 'where?'.
> 
> Users that really want to control their period (this could the case for
> the clk-pwm driver) could completely ignore DT/lookup-table information
> and set the period and absolute dutycycle directly.

Yes, I think clk-pwm is very special in this regard because the period
can be derived from the requested clock rate. It would be complicated to
implement DT parsing that ignores parts of the specifier in some cases
but not in others. Simply having the clk-pwm driver ignore whatever is
in the table (or perhaps bail out on periods other than 0 for example).

> Now, from what I seen, what most PWM users want to do is:
> 
> 	pwm_set_rel_duty_scale(pwm, rel_value, scale);
> or 
> 	rel_duty = pwm_get_rel_duty_cycle(pwm, scale);
> 
> where scale depends on the precision you need for your use case (most
> of the time it's expressed in percent).
> 
> So, how about providing this kind of API (this is what I proposed in
> one of my previous email)?
> 
> This would not only solve our problem (say you have a period at
> boot-time that differs from the one you'll set when first applying a
> new relative duty cycle, then the resulting relative value would still
> be correct), but it would also remove a lot of boiler plate code from
> PWM users code (if you take a look at pwm-regulator, pwm-leds, pwm-fan
> and probably others, you'll see that they are all doing this conversion
> manually).

I don't think this gains us much. The above would work for pwm-regulator
and pwm-fan, in both cases it'd replace a single line with two lines and
fitting the expressions into function arguments is likely going to be
hideous. For leds-pwm this wouldn't work, because of the low-active case
that it supports.

> Now, the last blocking point is, what if the PWM driver does not
> implement HW-readout. In this case, the pwm-regulator will probably
> expose a 0V output (IIRC, dutycycle is set to 0 by default) when it's
> actually providing something else. But is this really important? I
> mean, if the user really wants to have a reliable information, then he
> will implement initial-state retrieval in its PWM controller driver.
> Alternatively, we could put a flag specifying whether the PWM chip
> supports initial state retrieval.

I reached the same conclusion in another subthread. If hardware readout
isn't supported, I think the most natural thing to do is simply use the
initial state (i.e. what's defined in DT or board files) instead. There
is an argument, I think, to be made for having users apply the initial
state at probe time to forcibly apply the logical state to hardware and
subsequently not care about the initial state anymore. For most cases
that might not even be necessary, though.

Thierry

Attachment: signature.asc
Description: PGP signature