Re: [PATCH v3] pwm: lpss: Add get_state callback

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, 2017-02-28 at 15:09 +0100, Hans de Goede wrote:
> Hi,
> 
> On 27-02-17 23:20, Andy Shevchenko wrote:
> > On Mon, Feb 27, 2017 at 6:59 PM, Hans de Goede <hdegoede@redhat.com>
> > wrote:

> > > +       base_unit_range = BIT(lpwm->info->base_unit_bits);
> > > +
> > > +       ctrl = pwm_lpss_read(pwm);
> > > +       on_time_div = 255 - (ctrl & PWM_ON_TIME_DIV_MASK);
> > > +       base_unit = (ctrl >> PWM_BASE_UNIT_SHIFT) &
> > > (base_unit_range - 1);
> > > +
> > > +       freq = base_unit * lpwm->info->clk_rate;
> > > +       do_div(freq, base_unit_range);
> > > +       if (freq == 0)
> > > +               state->period = NSEC_PER_SEC;
> > 
> > And this is wrong. IIRC base_unit = 0 *or* on_time_div = 255 means
> > duty cycle = 100%.
> 
> Ok, so maybe we should do this then ?
> 
> 	if (freq == 0) {
> 		state->period = NSEC_PER_SEC;
> 		state->duty_cycle = NSEC_PER_SEC;
> 		return;
> 	}
> 
> ?

But the period is not that one. Can we assign them to 0?

> > > +       else
> > > +               state->period = NSEC_PER_SEC / (unsigned
> > > long)freq;
> > 
> > I would stop doing versions so fast and give one more try with
> > carefully chosen variable types and perhaps a bit straighter
> > arithmetic here.
> 
> I've carefully chosen the variable types, freq needs to
> be 64 bits for do_div usage. Here I'm going from
> freq (which has been divided by now) to a period time
> in nsec, for which:
> 
> 	state->period = NSEC_PER_SEC / (unsigned long)freq;
> 
> Is as straight forward as one can get, the
> (unsigned long) cast is there to force a 32 bit
> division since freq is known to fit in 32 bits at this
> time.

OK!

> 
> > > +
> > > +       on_time_div *= state->period;
> > > +       do_div(on_time_div, 255);
> > > +       state->duty_cycle = on_time_div;
> > > +
> > > +       state->polarity = PWM_POLARITY_NORMAL;
> > > +       state->enabled = !!(ctrl & PWM_ENABLE);
> > > +
> > > +       if (state->enabled)
> > > +               pm_runtime_get_sync(chip->dev);
> > 
> > Since you are too fast (again), let me ask where is the balanced put
> > for this call?
> 
> As I explained in my earlier mail this is balanced
> by the disable path in apply(). But I guess what you are
> getting at is what if the driver gets removed while the
> pwm is enabled? It seems that we then indeed leak our
> runtime-pm ref, note that is a pre-existing bug.

I see.

> Anyways since we now may get a pm-ref during probe
> I will add the balancing (and equally conditional)
> put of that ref to pwm_lpss_remove().
> 
> > Second question, how ->get_state() is supposed to work if power is
> > off
> > when it enters it?
> 
> get_state only gets called on probe and the device is guaranteed to
> be powered on during probe().

Ah, seems I now understand what this call back actually does.

So, the name of the callback a bit confusing, to me it sounds rather
"get_to_[initial_]state()".

Thus, would it be better to split ->apply() to get a common part for
both?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy