| Message ID | 20170227165941.1379-1-hdegoede@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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? > +}
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, };