* [PATCH v3] pwm: lpss: Add get_state callback
@ 2017-02-27 16:59 Hans de Goede
2017-02-27 22:20 ` Andy Shevchenko
0 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2017-02-27 16:59 UTC (permalink / raw)
To: Thierry Reding, Andy Shevchenko
Cc: Hans de Goede, linux-pwm, Takashi Iwai, Andy Shevchenko
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] pwm: lpss: Add get_state callback
2017-02-27 16:59 [PATCH v3] pwm: lpss: Add get_state callback Hans de Goede
@ 2017-02-27 22:20 ` Andy Shevchenko
2017-02-28 14:09 ` Hans de Goede
0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2017-02-27 22:20 UTC (permalink / raw)
To: Hans de Goede
Cc: Thierry Reding, Andy Shevchenko, linux-pwm, Takashi Iwai,
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] pwm: lpss: Add get_state callback
2017-02-27 22:20 ` Andy Shevchenko
@ 2017-02-28 14:09 ` Hans de Goede
2017-03-10 15:49 ` Andy Shevchenko
0 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2017-02-28 14:09 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Thierry Reding, Andy Shevchenko, linux-pwm, Takashi Iwai,
Andy Shevchenko
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] pwm: lpss: Add get_state callback
2017-02-28 14:09 ` Hans de Goede
@ 2017-03-10 15:49 ` Andy Shevchenko
2017-03-10 21:13 ` Hans de Goede
0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2017-03-10 15:49 UTC (permalink / raw)
To: Hans de Goede, Andy Shevchenko
Cc: Thierry Reding, Andy Shevchenko, linux-pwm, Takashi Iwai
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] pwm: lpss: Add get_state callback
2017-03-10 15:49 ` Andy Shevchenko
@ 2017-03-10 21:13 ` Hans de Goede
2017-03-26 12:34 ` Andy Shevchenko
0 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2017-03-10 21:13 UTC (permalink / raw)
To: Andy Shevchenko, Andy Shevchenko
Cc: Thierry Reding, Andy Shevchenko, linux-pwm, Takashi Iwai
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] pwm: lpss: Add get_state callback
2017-03-10 21:13 ` Hans de Goede
@ 2017-03-26 12:34 ` Andy Shevchenko
2017-03-26 14:46 ` Hans de Goede
0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2017-03-26 12:34 UTC (permalink / raw)
To: Hans de Goede, Andy Shevchenko
Cc: Thierry Reding, Andy Shevchenko, linux-pwm, Takashi Iwai
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] pwm: lpss: Add get_state callback
2017-03-26 12:34 ` Andy Shevchenko
@ 2017-03-26 14:46 ` Hans de Goede
0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2017-03-26 14:46 UTC (permalink / raw)
To: Andy Shevchenko, Andy Shevchenko
Cc: Thierry Reding, Andy Shevchenko, linux-pwm, Takashi Iwai
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-26 14:46 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-27 16:59 [PATCH v3] pwm: lpss: Add get_state callback Hans de Goede
2017-02-27 22:20 ` Andy Shevchenko
2017-02-28 14:09 ` Hans de Goede
2017-03-10 15:49 ` Andy Shevchenko
2017-03-10 21:13 ` Hans de Goede
2017-03-26 12:34 ` Andy Shevchenko
2017-03-26 14:46 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.