[PATCH v3] pwm: lpss: Add get_state callback

[PATCH v3] pwm: lpss: Add get_state callback

[Hans de Goede]

Add a get_state callback so that the initial state correctly reflects
the actual hardware state.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
Changes in v2:
-Rebase on top of linux-pwm/for-next
Changes in v3:
-Invert on_time_div 255 means 0 brightness not max brightness
-Use do_div to avoid issues with 64 bit division on 32 bit archs
-Use !!foo instead of foo? true:fale
---
 drivers/pwm/pwm-lpss.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 0b549dc..6605777 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -154,8 +154,41 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return ret;
 }
 
+static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			       struct pwm_state *state)
+{
+	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
+	unsigned long base_unit_range;
+	unsigned long long base_unit, freq, on_time_div;
+	u32 ctrl;
+
+	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;
+	else
+		state->period = NSEC_PER_SEC / (unsigned long)freq;
+
+	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);
+}
+
 static const struct pwm_ops pwm_lpss_ops = {
 	.apply = pwm_lpss_apply,
+	.get_state = pwm_lpss_get_state,
 	.owner = THIS_MODULE,
 };
 
-- 
2.9.3

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

[Andy Shevchenko]

On Mon, Feb 27, 2017 at 6:59 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Add a get_state callback so that the initial state correctly reflects
> the actual hardware state.

> +static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +                              struct pwm_state *state)
> +{
> +       struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> +       unsigned long base_unit_range;
> +       unsigned long long base_unit, freq, on_time_div;
> +       u32 ctrl;
> +
> +       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%.

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

> +
> +       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?
Second question, how ->get_state() is supposed to work if power is off
when it enters it?

> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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:
>> Add a get_state callback so that the initial state correctly reflects
>> the actual hardware state.
>
>> +static void pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> +                              struct pwm_state *state)
>> +{
>> +       struct pwm_lpss_chip *lpwm = to_lpwm(chip);
>> +       unsigned long base_unit_range;
>> +       unsigned long long base_unit, freq, on_time_div;
>> +       u32 ctrl;
>> +
>> +       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;
	}

?


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

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

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

Regards,

Hans

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

[Andy Shevchenko]

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

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

[Hans de Goede]

Hi,

On 03/10/2017 04:49 PM, Andy Shevchenko wrote:
> 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?

That will cause a divide by 0 in apply() when a user
calls apply without changing / setting period.

>
>>>> +       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?

I don't understand what you mean here ? Ah I guess you mean split
out the pm_runtime_get / put stuff. I don't think that is going
to work well as apply compares old and new state and we do not
have old state when get_state gets called.

Regards,

Hans



>

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

[Andy Shevchenko]

On Fri, 2017-03-10 at 22:13 +0100, Hans de Goede wrote:
> Hi,
> 
> On 03/10/2017 04:49 PM, Andy Shevchenko wrote:
> > 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:

> > > 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?
> 
> That will cause a divide by 0 in apply() when a user
> calls apply without changing / setting period.

I have to look closer to this one. Perhaps next week together with yours
other change.

> > > 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?
> 
> I don't understand what you mean here ? Ah I guess you mean split
> out the pm_runtime_get / put stuff. I don't think that is going
> to work well as apply compares old and new state and we do not
> have old state when get_state gets called.

I mean to have similar flow in ->get_state() and ->apply() when we
enable the device.

I don't like ->get_state() name as I mentioned before. If its
functionality is "get *to* (initial) state", that what we need to
implement.

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

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

[Hans de Goede]

Hi,

On 26-03-17 14:34, Andy Shevchenko wrote:
> On Fri, 2017-03-10 at 22:13 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 03/10/2017 04:49 PM, Andy Shevchenko wrote:
>>> 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:
>
>>>> 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?
>>
>> That will cause a divide by 0 in apply() when a user
>> calls apply without changing / setting period.
>
> I have to look closer to this one. Perhaps next week together with yours
> other change.

Ok, note this one has no hurry.

>>>> 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?
>>
>> I don't understand what you mean here ? Ah I guess you mean split
>> out the pm_runtime_get / put stuff. I don't think that is going
>> to work well as apply compares old and new state and we do not
>> have old state when get_state gets called.
>
> I mean to have similar flow in ->get_state() and ->apply() when we
> enable the device.
>
> I don't like ->get_state() name as I mentioned before. If its
> functionality is "get *to* (initial) state", that what we need to
> implement.

The way this works is part of the pwm core, also I still do not
understand what you're trying to say here, can you give some
pseudo-code as example how you want to see things split ?

Regards,

Hans