Messages in this thread | | | From | Mathieu Poirier <> | Date | Fri, 27 Sep 2019 12:02:49 -0600 | Subject | Re: [PATCH 3/3] powercap/drivers/idle_inject: Specify the idle state to inject |
| |
On Tue, 24 Sep 2019 at 07:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > > Hi Mathieu, > > On 18/09/2019 21:35, Mathieu Poirier wrote: > > On Mon, Sep 09, 2019 at 04:50:15PM +0200, Daniel Lezcano wrote: > >> Currently the idle injection framework only allows to inject the > >> deepest idle state available on the system. > >> > >> Give the opportunity to specify which idle state we want to inject by > >> adding a new function helper to set the state and use it when calling > >> play_idle(). > >> > >> There is no functional changes, the cpuidle state is the deepest one. > >> > >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >> --- > > [ ... ] > > >> +/** > >> + * idle_inject_set_state - set the idle state to inject > >> + * @state: an integer for the idle state to inject > >> + */ > >> +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state) > >> +{ > >> + if (state >= CPUIDLE_STATE_NOUSE && state < CPUIDLE_STATE_MAX) > >> + WRITE_ONCE(ii_dev->state, state); > >> +} > >> + > >> /** > >> * idle_inject_start - start idle injections > >> * @ii_dev: idle injection control device structure > >> @@ -298,6 +310,7 @@ struct idle_inject_device *idle_inject_register(struct cpumask *cpumask) > >> cpumask_copy(to_cpumask(ii_dev->cpumask), cpumask); > >> hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > >> ii_dev->timer.function = idle_inject_timer_fn; > >> + ii_dev->state = cpuidle_find_deepest_state(); > >> > >> for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) { > >> > >> diff --git a/include/linux/idle_inject.h b/include/linux/idle_inject.h > >> index a445cd1a36c5..e2b26b9ccd34 100644 > >> --- a/include/linux/idle_inject.h > >> +++ b/include/linux/idle_inject.h > >> @@ -26,4 +26,7 @@ void idle_inject_set_duration(struct idle_inject_device *ii_dev, > >> void idle_inject_get_duration(struct idle_inject_device *ii_dev, > >> unsigned int *run_duration_us, > >> unsigned int *idle_duration_us); > >> + > >> +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state); > >> + > > > > The above function is not used in this patch and as such should be introduce as > > part of future work. Otherwise I agree that this patch does not carry any > > functional changes. > > > > Without function idle_inject_set_state(): > > I was about to remove the function but actually it may not make sense as > the idle_inject is a framework providing the different API to do the > idle injection and the function is an helper to set the state value. It > comes with the addition of the state number in the structure.
I agree with you but there is still no current user of the API.
> > Next patch is the idle cooling device and makes use of it.
Then why not simply introduce it then?
> > Can I still consider your acked-by valid?
Whether the API is introduced here or in the next patchset has no effect on the validity of the code and as such my approbation is still valid.
Thanks, Mathieu
> > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog >
| |