* [PATCH 1/5] PM / Domains: Split device PM domain data into base and need_restore
From: Rafael J. Wysocki @ 2011-08-30 22:18 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: LKML, Linux-sh list
In-Reply-To: <201108310017.03103.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
The struct pm_domain_data data type is defined in such a way that
adding new fields specific to the generic PM domains code will
require include/linux/pm.h to be modified. As a result, data types
used only by the generic PM domains code will be defined in two
headers, although they all should be defined in pm_domain.h and
pm.h will need to include more headers, which won't be very nice.
For this reason change the definition of struct pm_subsys_data
so that its domain_data member is a pointer, which will allow
struct pm_domain_data to be subclassed by various PM domains
implementations. Remove the need_restore member from
struct pm_domain_data and make the generic PM domains code
subclass it by adding the need_restore member to the new data type.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
drivers/base/power/domain.c | 28 +++++++++++++++++++---------
include/linux/pm.h | 3 +--
include/linux/pm_domain.h | 10 ++++++++++
3 files changed, 30 insertions(+), 11 deletions(-)
Index: linux/include/linux/pm.h
===================================================================
--- linux.orig/include/linux/pm.h
+++ linux/include/linux/pm.h
@@ -433,7 +433,6 @@ struct wakeup_source;
struct pm_domain_data {
struct list_head list_node;
struct device *dev;
- bool need_restore;
};
struct pm_subsys_data {
@@ -443,7 +442,7 @@ struct pm_subsys_data {
struct list_head clock_list;
#endif
#ifdef CONFIG_PM_GENERIC_DOMAINS
- struct pm_domain_data domain_data;
+ struct pm_domain_data *domain_data;
#endif
};
Index: linux/include/linux/pm_domain.h
===================================================================
--- linux.orig/include/linux/pm_domain.h
+++ linux/include/linux/pm_domain.h
@@ -62,6 +62,16 @@ struct gpd_link {
struct list_head slave_node;
};
+struct generic_pm_domain_data {
+ struct pm_domain_data base;
+ bool need_restore;
+};
+
+static inline struct generic_pm_domain_data *to_gpd_data(struct pm_domain_data *pdd)
+{
+ return container_of(pdd, struct generic_pm_domain_data, base);
+}
+
#ifdef CONFIG_PM_GENERIC_DOMAINS
extern int pm_genpd_add_device(struct generic_pm_domain *genpd,
struct device *dev);
Index: linux/drivers/base/power/domain.c
===================================================================
--- linux.orig/drivers/base/power/domain.c
+++ linux/drivers/base/power/domain.c
@@ -188,11 +188,12 @@ static int __pm_genpd_save_device(struct
struct generic_pm_domain *genpd)
__releases(&genpd->lock) __acquires(&genpd->lock)
{
+ struct generic_pm_domain_data *gpd_data = to_gpd_data(pdd);
struct device *dev = pdd->dev;
struct device_driver *drv = dev->driver;
int ret = 0;
- if (pdd->need_restore)
+ if (gpd_data->need_restore)
return 0;
mutex_unlock(&genpd->lock);
@@ -210,7 +211,7 @@ static int __pm_genpd_save_device(struct
mutex_lock(&genpd->lock);
if (!ret)
- pdd->need_restore = true;
+ gpd_data->need_restore = true;
return ret;
}
@@ -224,10 +225,11 @@ static void __pm_genpd_restore_device(st
struct generic_pm_domain *genpd)
__releases(&genpd->lock) __acquires(&genpd->lock)
{
+ struct generic_pm_domain_data *gpd_data = to_gpd_data(pdd);
struct device *dev = pdd->dev;
struct device_driver *drv = dev->driver;
- if (!pdd->need_restore)
+ if (!gpd_data->need_restore)
return;
mutex_unlock(&genpd->lock);
@@ -244,7 +246,7 @@ static void __pm_genpd_restore_device(st
mutex_lock(&genpd->lock);
- pdd->need_restore = false;
+ gpd_data->need_restore = false;
}
/**
@@ -493,7 +495,7 @@ static int pm_genpd_runtime_resume(struc
mutex_lock(&genpd->lock);
}
finish_wait(&genpd->status_wait_queue, &wait);
- __pm_genpd_restore_device(&dev->power.subsys_data->domain_data, genpd);
+ __pm_genpd_restore_device(dev->power.subsys_data->domain_data, genpd);
genpd->resume_count--;
genpd_set_active(genpd);
wake_up_all(&genpd->status_wait_queue);
@@ -1080,6 +1082,7 @@ static void pm_genpd_complete(struct dev
*/
int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
{
+ struct generic_pm_domain_data *gpd_data;
struct pm_domain_data *pdd;
int ret = 0;
@@ -1106,14 +1109,20 @@ int pm_genpd_add_device(struct generic_p
goto out;
}
+ gpd_data = kzalloc(sizeof(*gpd_data), GFP_KERNEL);
+ if (!gpd_data) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
genpd->device_count++;
dev->pm_domain = &genpd->domain;
dev_pm_get_subsys_data(dev);
- pdd = &dev->power.subsys_data->domain_data;
- pdd->dev = dev;
- pdd->need_restore = false;
- list_add_tail(&pdd->list_node, &genpd->dev_list);
+ dev->power.subsys_data->domain_data = &gpd_data->base;
+ gpd_data->base.dev = dev;
+ gpd_data->need_restore = false;
+ list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
out:
genpd_release_lock(genpd);
@@ -1152,6 +1161,7 @@ int pm_genpd_remove_device(struct generi
pdd->dev = NULL;
dev_pm_put_subsys_data(dev);
dev->pm_domain = NULL;
+ kfree(to_gpd_data(pdd));
genpd->device_count--;
^ permalink raw reply
* [PATCH 0/5] PM: Generic PM domains and device PM QoS
From: Rafael J. Wysocki @ 2011-08-30 22:17 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: LKML, Linux-sh list
Hi,
This patchset illustrates how device PM QoS may be used along with
PM domains in my view.
Actually, it consists of two parts. Namely, patches [1-3/5] seem to be
suitable for 3.2, unless somebody hates them, but patches [4-5/5] are
total RFC. They haven't been tested, only compiled, so the use of them
is not encouraged (they may kill your dog or make your cat go wild, or
do something equally nasty, so beware). Their purpose is to illustrate
an idea that I'd like to discuss at the PM miniconference during the
LPC.
[1/5] - Split device PM domain data into a "base" and additional fields
(one need_restore field at the moment, but the subsequent patches
add more fields).
[2/5] - Make runtime PM always release power.lock if power.irq_safe is set.
[3/5] - Add function for reading device PM QoS values safely.
[4/5] - Add governor function for stopping devices.
[5/5] - Add generic PM domain power off governor function.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 6/6] cgroup: kill subsys->can_attach_task(), pre_attach() and attach_task()
From: Frederic Weisbecker @ 2011-08-30 20:10 UTC (permalink / raw)
To: Tejun Heo; +Cc: containers, lizf, linux-kernel, linux-pm, paul, kamezawa.hiroyu
In-Reply-To: <1314312192-26885-7-git-send-email-tj@kernel.org>
On Fri, Aug 26, 2011 at 12:43:12AM +0200, Tejun Heo wrote:
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index f3c7f7a..ce765ec 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2075,7 +2064,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> {
> int retval, i, group_size, nr_migrating_tasks;
> struct cgroup_subsys *ss, *failed_ss = NULL;
> - bool cancel_failed_ss = false;
> /* guaranteed to be initialized later, but the compiler needs this */
> struct css_set *oldcg;
> struct cgroupfs_root *root = cgrp->root;
> @@ -2166,21 +2154,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> goto out_cancel_attach;
> }
> }
> - /* a callback to be run on every thread in the threadgroup. */
> - if (ss->can_attach_task) {
> - /* run on each task in the threadgroup. */
> - for (i = 0; i < group_size; i++) {
> - tc = flex_array_get(group, i);
> - if (tc->cgrp == cgrp)
> - continue;
> - retval = ss->can_attach_task(cgrp, tc->task);
> - if (retval) {
> - failed_ss = ss;
> - cancel_failed_ss = true;
> - goto out_cancel_attach;
> - }
> - }
> - }
> }
>
> /*
> @@ -2217,15 +2190,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> }
>
> /*
> - * step 3: now that we're guaranteed success wrt the css_sets, proceed
> - * to move all tasks to the new cgroup, calling ss->attach_task for each
> - * one along the way. there are no failure cases after here, so this is
> - * the commit point.
> + * step 3: now that we're guaranteed success wrt the css_sets,
> + * proceed to move all tasks to the new cgroup. There are no
> + * failure cases after here, so this is the commit point.
> */
> - for_each_subsys(root, ss) {
> - if (ss->pre_attach)
> - ss->pre_attach(cgrp);
> - }
> for (i = 0; i < group_size; i++) {
> tc = flex_array_get(group, i);
> /* leave current thread as it is if it's already there */
> @@ -2235,19 +2203,11 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> /* if the thread is PF_EXITING, it can just get skipped. */
> retval = cgroup_task_migrate(cgrp, tc->cgrp, tc->task, true);
> BUG_ON(retval != 0 && retval != -ESRCH);
> -
> - /* attach each task to each subsystem */
> - for_each_subsys(root, ss) {
> - if (ss->attach_task)
> - ss->attach_task(cgrp, tc->task);
> - }
In order to keep the fix queued in -mm (https://lkml.org/lkml/2011/8/26/262)
the tasks that have failed to migrate should be removed from the iterator
so that they are not included in the batch in ->attach().
> }
> /* nothing is sensitive to fork() after this point. */
>
> /*
> - * step 4: do expensive, non-thread-specific subsystem callbacks.
> - * TODO: if ever a subsystem needs to know the oldcgrp for each task
> - * being moved, this call will need to be reworked to communicate that.
> + * step 4: do subsystem attach callbacks.
> */
> for_each_subsys(root, ss) {
> if (ss->attach)
> @@ -2271,11 +2231,8 @@ out_cancel_attach:
> /* same deal as in cgroup_attach_task */
> if (retval) {
> for_each_subsys(root, ss) {
> - if (ss == failed_ss) {
> - if (cancel_failed_ss && ss->cancel_attach)
> - ss->cancel_attach(ss, cgrp, &tset);
> + if (ss == failed_ss)
> break;
> - }
> if (ss->cancel_attach)
> ss->cancel_attach(ss, cgrp, &tset);
> }
> --
> 1.7.6
>
^ permalink raw reply
* Re: [PATCH v8 3/5] PM / DEVFREQ: add common sysfs interfaces
From: Turquette, Mike @ 2011-08-30 17:10 UTC (permalink / raw)
To: myungjoo.ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <CAJ0PZbQY0Li-uYQGxFqzjuuzCR4ViNy2AHE=8SiNCbADoG8z=g@mail.gmail.com>
On Mon, Aug 29, 2011 at 9:28 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> On Tue, Aug 30, 2011 at 4:17 AM, Turquette, Mike <mturquette@ti.com> wrote:
>>
>> Sorry I'll have to rescind my ACK. I just reviewed patch 5/5 and the
>> show_freq and restore_freq functions for the userspace governor do not
>> protect the private data accesses with a mutex. Looking a bit further
>> I see that they do call get_devfreq; I think the semantics for this
>> are wrong.
>>
>> get_devfreq only protects the list as long as it takes to do a lookup.
>> However neither struct devfreq or struct userspace_data have a mutex
>> associated with them, so concurrent operations are not protected.
>>
>> One heavy-handed way of solving this is to not have get_devfreq
>> release the mutex, and then have those functions call a new
>> put_devfreq which does release the mutex. However that is really bad
>> locking.
>>
>> What do you think about putting a mutex in struct devfreq? The rule
>> for governors can be that access to the governor-specific private data
>> requires taking that devfreq's mutex.
>
>
> A lock in private data at DEVFREQ framework won't be needed as it is a
> governor-specific issue; however, it appears that we need a locking
> mechanism for accessing struct devfreq in general for governors.
> Although we do not have any governor that writes something or reads
> multiple times on struct devfreq (except for the private data) out of
> get_target_freq ops, which does not need an additional lock, we may
> have one soon.
>
> Then, I will need to update the DEVFREQ core as well.
> There will be two locks in devfreq.c: the global devfreq_list_lock and
> per-devfreq devfreq_lock in struct devfreq.
> And the role of devfreq_lock will be protecting the access to struct
> devfreq (some functions in devfreq.c will use this lock and some usage
> of devfreq_list_lock will be removed).
>
> This will result in more general sysfs interface (or functions other
> than the standard ops) support in governors.
Sounds good. Also, please add a comment somewhere in devfreq.h
(probably above definition for struct devfreq) that the lock must also
be held by governors when accessing devfreq->data.
Regards,
Mike
> Thank you so much!
>
>
> Cheers,
> MyungJoo.
>
>>
>> Regards,
>> Mike
>>
>>> +
>>> +/**
>>> * devfreq_do() - Check the usage profile of a given device and configure
>>> * frequency and voltage accordingly
>>> * @devfreq: devfreq info of the given device
>>> @@ -149,6 +167,8 @@ static void devfreq_monitor(struct work_struct *work)
>>> dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
>>> error, devfreq->governor->name);
>>>
>>> + sysfs_unmerge_group(&devfreq->dev->kobj,
>>> + &dev_attr_group);
>>> list_del(&devfreq->node);
>>> kfree(devfreq);
>>>
>>> @@ -239,6 +259,8 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
>>> queue_delayed_work(devfreq_wq, &devfreq_work,
>>> devfreq->next_polling);
>>> }
>>> +
>>> + sysfs_merge_group(&dev->kobj, &dev_attr_group);
>>> out:
>>> mutex_unlock(&devfreq_list_lock);
>>>
>>> @@ -271,6 +293,8 @@ int devfreq_remove_device(struct device *dev)
>>> goto out;
>>> }
>>>
>>> + sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
>>> +
>>> list_del(&devfreq->node);
>>> srcu_notifier_chain_unregister(nh, &devfreq->nb);
>>> kfree(devfreq);
>>> @@ -279,6 +303,132 @@ out:
>>> return 0;
>>> }
>>>
>>> +static ssize_t show_governor(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->governor)
>>> + return -EINVAL;
>>> +
>>> + return sprintf(buf, "%s\n", df->governor->name);
>>> +}
>>> +
>>> +static ssize_t show_freq(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> +
>>> + return sprintf(buf, "%lu\n", df->previous_freq);
>>> +}
>>> +
>>> +static ssize_t show_max_freq(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> + unsigned long freq = ULONG_MAX;
>>> + struct opp *opp;
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->dev)
>>> + return -EINVAL;
>>> +
>>> + opp = opp_find_freq_floor(df->dev, &freq);
>>> + if (IS_ERR(opp))
>>> + return PTR_ERR(opp);
>>> +
>>> + return sprintf(buf, "%lu\n", freq);
>>> +}
>>> +
>>> +static ssize_t show_min_freq(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> + unsigned long freq = 0;
>>> + struct opp *opp;
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->dev)
>>> + return -EINVAL;
>>> +
>>> + opp = opp_find_freq_ceil(df->dev, &freq);
>>> + if (IS_ERR(opp))
>>> + return PTR_ERR(opp);
>>> +
>>> + return sprintf(buf, "%lu\n", freq);
>>> +}
>>> +
>>> +static ssize_t show_polling_interval(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->profile)
>>> + return -EINVAL;
>>> +
>>> + return sprintf(buf, "%d\n", df->profile->polling_ms);
>>> +}
>>> +
>>> +static ssize_t store_polling_interval(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct devfreq *df = get_devfreq(dev);
>>> + unsigned int value;
>>> + int ret;
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->profile)
>>> + return -EINVAL;
>>> +
>>> + ret = sscanf(buf, "%u", &value);
>>> + if (ret != 1)
>>> + return -EINVAL;
>>> +
>>> + df->profile->polling_ms = value;
>>> + df->next_polling = df->polling_jiffies
>>> + = msecs_to_jiffies(value);
>>> +
>>> + mutex_lock(&devfreq_list_lock);
>>> + if (df->next_polling > 0 && !polling) {
>>> + polling = true;
>>> + queue_delayed_work(devfreq_wq, &devfreq_work,
>>> + df->next_polling);
>>> + }
>>> + mutex_unlock(&devfreq_list_lock);
>>> +
>>> + return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR(devfreq_governor, 0444, show_governor, NULL);
>>> +static DEVICE_ATTR(devfreq_cur_freq, 0444, show_freq, NULL);
>>> +static DEVICE_ATTR(devfreq_max_freq, 0444, show_max_freq, NULL);
>>> +static DEVICE_ATTR(devfreq_min_freq, 0444, show_min_freq, NULL);
>>> +static DEVICE_ATTR(devfreq_polling_interval, 0644, show_polling_interval,
>>> + store_polling_interval);
>>> +static struct attribute *dev_entries[] = {
>>> + &dev_attr_devfreq_governor.attr,
>>> + &dev_attr_devfreq_cur_freq.attr,
>>> + &dev_attr_devfreq_max_freq.attr,
>>> + &dev_attr_devfreq_min_freq.attr,
>>> + &dev_attr_devfreq_polling_interval.attr,
>>> + NULL,
>>> +};
>>> +static struct attribute_group dev_attr_group = {
>>> + .name = power_group_name,
>>> + .attrs = dev_entries,
>>> +};
>>> +
>>> /**
>>> * devfreq_init() - Initialize data structure for devfreq framework and
>>> * start polling registered devfreq devices.
>>> --
>>> 1.7.4.1
>>>
>>>
>>
>
>
>
> --
> MyungJoo Ham (함명주), Ph.D.
> Mobile Software Platform Lab,
> Digital Media and Communications (DMC) Business
> Samsung Electronics
> cell: 82-10-6714-2858
>
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH v8 5/5] PM / DEVFREQ: add basic governors
From: Turquette, Mike @ 2011-08-30 17:09 UTC (permalink / raw)
To: myungjoo.ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <CAJ0PZbQcfg+e9D3Ne-U4kyAbR4p2-Ua0bJ=sq7Ko9sCkjooAbw@mail.gmail.com>
On Mon, Aug 29, 2011 at 9:19 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> On Tue, Aug 30, 2011 at 3:58 AM, Turquette, Mike <mturquette@ti.com> wrote:
>> On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>>> +What: /sys/devices/.../power/devfreq_userspace_set_freq
>>
>> How about just .../devfreq_set_freq? I think the userspace bit is
>> implied and the name is very long.
>>
>
> Umm.. as this entry became userspace specific, I though I need it be
> to distinguished from entries created by devfreq framework. However,
> this one is created only while userspace is being used (device may
> replace governors by calling remove and add); thus, there wouldn't be
> any duplicated name issues. So, I think removing userspace from the
> name should be fine. I will do so in the next revision.
It is your choice. You have a good point that it is specific to the
"userspace" governor. I hadn't thought of that at the time, so I
don't care either way.
Regards,
Mike
>>> +
>>> +static ssize_t show_freq(struct device *dev, struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct devfreq *devfreq = get_devfreq(dev);
>>> + struct userspace_data *data;
>>> + int err = 0;
>>> +
>>> + if (IS_ERR(devfreq)) {
>>> + err = PTR_ERR(devfreq);
>>> + goto out;
>>> + }
>>> + data = devfreq->data;
>>> +
>>> + if (data->valid)
>>> + err = sprintf(buf, "%lu\n", data->user_frequency);
>>> + else
>>> + err = sprintf(buf, "undefined\n");
>>> +out:
>>> + return err;
>>> +}
>>
>> Shouldn't accesses to devfreq->data be protected by a mutex?
>>
>> Regards,
>> Mike
>
>
> No, it doesn't need mutex here. Although generally, a mutex will be
> needed for simliar codes, in this specific case, devfreq->data does
> not need a mutex protection.
>
> There are two variables in data: "user_frequency" and "valid"
> - valid is initially false and becomes true at store_freq() and never changes.
> - store_freq() writes user_frequency and then writes valid as true.
> (no one writes false on valid)
> - user_frequency is read only when valid is true.
>
> Thus, the case where mutex protection serializes with some dictinction is:
> 1. Init. (valid = false)
> 2. store_freq() writes user_frequency = X
> 3. show_freq()/devfreq_userspace_func() reads valid (false)
> 4. store_freq() writes valid = true
> 5. show_freq()/devfreq_userspace_func() returns without reading
> devfreq_userspace_func()
> 6. store_freq() returns.
>
> into
> A.
> 1. Init (valid = false)
> 2. store_freq() starts and finishes
> 3. show_freq()/devfreq_userspace_func() starts and finishes
> B.
> 1. Init (valid = false)
> 2. show_freq()/devfreq_userspace_func() starts and finishes
> 3. store_freq() starts and finishes
>
> where B results in the same thing as non-mutex version does.
>
>
> If there is an operation that makes valid false, we will need a mutex
> (local to the governor) anyway.
>
>
>
> Anyway, I agree with you on the point that governors might need a
> locking mechanism in general; not just on the private data, but on the
> devfreq access. I'll put more on this issue on the other thread.
>
>
>
> Thank you.
>
>
> Cheers,
> MyungJoo
>
>
>>
>>> +
>>> +static DEVICE_ATTR(devfreq_userspace_set_freq, 0644, show_freq, store_freq);
>>> +static struct attribute *dev_entries[] = {
>>> + &dev_attr_devfreq_userspace_set_freq.attr,
>>> + NULL,
>>> +};
>>> +static struct attribute_group dev_attr_group = {
>>> + .name = power_group_name,
>>> + .attrs = dev_entries,
>>> +};
>>> +
>>> +static int userspace_init(struct devfreq *devfreq)
>>> +{
>>> + int err = 0;
>>> + struct userspace_data *data = kzalloc(sizeof(struct userspace_data),
>>> + GFP_KERNEL);
>>> +
>>> + if (!data) {
>>> + err = -ENOMEM;
>>> + goto out;
>>> + }
>>> + data->valid = false;
>>> + devfreq->data = data;
>>> +
>>> + sysfs_merge_group(&devfreq->dev->kobj, &dev_attr_group);
>>> +out:
>>> + return err;
>>> +}
>>> +
>>> +static void userspace_exit(struct devfreq *devfreq)
>>> +{
>>> + sysfs_unmerge_group(&devfreq->dev->kobj, &dev_attr_group);
>>> + kfree(devfreq->data);
>>> + devfreq->data = NULL;
>>> +}
>>> +
>>> +struct devfreq_governor devfreq_userspace = {
>>> + .name = "userspace",
>>> + .get_target_freq = devfreq_userspace_func,
>>> + .init = userspace_init,
>>> + .exit = userspace_exit,
>>> +};
>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>>> index fdc6916..cbafcdf 100644
>>> --- a/include/linux/devfreq.h
>>> +++ b/include/linux/devfreq.h
>>> @@ -13,6 +13,7 @@
>>> #ifndef __LINUX_DEVFREQ_H__
>>> #define __LINUX_DEVFREQ_H__
>>>
>>> +#include <linux/opp.h>
>>> #include <linux/notifier.h>
>>>
>>> #define DEVFREQ_NAME_LEN 16
>>> @@ -65,6 +66,8 @@ struct devfreq_governor {
>>> * "devfreq_monitor" executions to reevaluate
>>> * frequency/voltage of the device. Set by
>>> * profile's polling_ms interval.
>>> + * @user_set_freq User specified adequete frequency value (thru sysfs
>>> + * interface). Governors may and may not use this value.
>>> * @data Private data of the governor. The devfreq framework does not
>>> * touch this.
>>> *
>>> @@ -82,6 +85,7 @@ struct devfreq {
>>> unsigned long previous_freq;
>>> unsigned int next_polling;
>>>
>>> + unsigned long user_set_freq; /* governors may ignore this. */
>>> void *data; /* private data for governors */
>>> };
>>>
>>> @@ -91,6 +95,37 @@ extern int devfreq_add_device(struct device *dev,
>>> struct devfreq_governor *governor,
>>> void *data);
>>> extern int devfreq_remove_device(struct device *dev);
>>> +
>>> +#ifdef CONFIG_DEVFREQ_GOV_POWERSAVE
>>> +extern struct devfreq_governor devfreq_powersave;
>>> +#endif
>>> +#ifdef CONFIG_DEVFREQ_GOV_PERFORMANCE
>>> +extern struct devfreq_governor devfreq_performance;
>>> +#endif
>>> +#ifdef CONFIG_DEVFREQ_GOV_USERSPACE
>>> +extern struct devfreq_governor devfreq_userspace;
>>> +#endif
>>> +#ifdef CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND
>>> +extern struct devfreq_governor devfreq_simple_ondemand;
>>> +/**
>>> + * struct devfreq_simple_ondemand_data - void *data fed to struct devfreq
>>> + * and devfreq_add_device
>>> + * @ upthreshold If the load is over this value, the frequency jumps.
>>> + * Specify 0 to use the default. Valid value = 0 to 100.
>>> + * @ downdifferential If the load is under upthreshold - downdifferential,
>>> + * the governor may consider slowing the frequency down.
>>> + * Specify 0 to use the default. Valid value = 0 to 100.
>>> + * downdifferential < upthreshold must hold.
>>> + *
>>> + * If the fed devfreq_simple_ondemand_data pointer is NULL to the governor,
>>> + * the governor uses the default values.
>>> + */
>>> +struct devfreq_simple_ondemand_data {
>>> + unsigned int upthreshold;
>>> + unsigned int downdifferential;
>>> +};
>>> +#endif
>>> +
>>> #else /* !CONFIG_PM_DEVFREQ */
>>> static int devfreq_add_device(struct device *dev,
>>> struct devfreq_dev_profile *profile,
>>> @@ -104,6 +139,12 @@ static int devfreq_remove_device(struct device *dev)
>>> {
>>> return 0;
>>> }
>>> +
>>> +#define devfreq_powersave NULL
>>> +#define devfreq_performance NULL
>>> +#define devfreq_userspace NULL
>>> +#define devfreq_simple_ondemand NULL
>>> +
>>> #endif /* CONFIG_PM_DEVFREQ */
>>>
>>> #endif /* __LINUX_DEVFREQ_H__ */
>>> --
>>> 1.7.4.1
>>>
>>>
>>
>
>
>
> --
> MyungJoo Ham (함명주), Ph.D.
> Mobile Software Platform Lab,
> Digital Media and Communications (DMC) Business
> Samsung Electronics
> cell: 82-10-6714-2858
>
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH v2 4/4] bq24022: Add support for the bq2407x family
From: Mark Brown @ 2011-08-30 10:52 UTC (permalink / raw)
To: Heiko Stübner; +Cc: linux-pm, Liam Girdwood, Philipp Zabel
In-Reply-To: <201108291759.35396.heiko@sntech.de>
On Mon, Aug 29, 2011 at 05:59:35PM +0200, Heiko Stübner wrote:
> The bq2407x family of charger ICs simply supports another machine
> specific charging current above 500mA. To use this setting a
> second gpio pin is used while the rest of the pins stay the same.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
^ permalink raw reply
* Re: [PATCH 2/4] bq24022: Use dev_err instead of dev_dbg for error messages
From: Mark Brown @ 2011-08-30 10:40 UTC (permalink / raw)
To: Heiko Stübner; +Cc: linux-pm, Liam Girdwood, Philipp Zabel
In-Reply-To: <201108291757.30189.heiko@sntech.de>
On Mon, Aug 29, 2011 at 05:57:29PM +0200, Heiko Stübner wrote:
> This makes error messages visible to the user,
> so he/she can eventually fix them.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
^ permalink raw reply
* Re: [PATCH v2 1/4] bq24022: Evaluate returns of gpio_direction_output-calls
From: Mark Brown @ 2011-08-30 10:39 UTC (permalink / raw)
To: Heiko Stübner; +Cc: linux-pm, Liam Girdwood, Philipp Zabel
In-Reply-To: <201108291756.17516.heiko@sntech.de>
On Mon, Aug 29, 2011 at 05:56:17PM +0200, Heiko Stübner wrote:
> It wasn't done before.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
^ permalink raw reply
* Re: [PATCH v8 3/5] PM / DEVFREQ: add common sysfs interfaces
From: MyungJoo Ham @ 2011-08-30 4:28 UTC (permalink / raw)
To: Turquette, Mike
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <CAJOA=zNHSC0kBMhRw1uf=8E7PE2Da6wXAWnG=mgh2M8-pa6w6g@mail.gmail.com>
On Tue, Aug 30, 2011 at 4:17 AM, Turquette, Mike <mturquette@ti.com> wrote:
>
> Sorry I'll have to rescind my ACK. I just reviewed patch 5/5 and the
> show_freq and restore_freq functions for the userspace governor do not
> protect the private data accesses with a mutex. Looking a bit further
> I see that they do call get_devfreq; I think the semantics for this
> are wrong.
>
> get_devfreq only protects the list as long as it takes to do a lookup.
> However neither struct devfreq or struct userspace_data have a mutex
> associated with them, so concurrent operations are not protected.
>
> One heavy-handed way of solving this is to not have get_devfreq
> release the mutex, and then have those functions call a new
> put_devfreq which does release the mutex. However that is really bad
> locking.
>
> What do you think about putting a mutex in struct devfreq? The rule
> for governors can be that access to the governor-specific private data
> requires taking that devfreq's mutex.
A lock in private data at DEVFREQ framework won't be needed as it is a
governor-specific issue; however, it appears that we need a locking
mechanism for accessing struct devfreq in general for governors.
Although we do not have any governor that writes something or reads
multiple times on struct devfreq (except for the private data) out of
get_target_freq ops, which does not need an additional lock, we may
have one soon.
Then, I will need to update the DEVFREQ core as well.
There will be two locks in devfreq.c: the global devfreq_list_lock and
per-devfreq devfreq_lock in struct devfreq.
And the role of devfreq_lock will be protecting the access to struct
devfreq (some functions in devfreq.c will use this lock and some usage
of devfreq_list_lock will be removed).
This will result in more general sysfs interface (or functions other
than the standard ops) support in governors.
Thank you so much!
Cheers,
MyungJoo.
>
> Regards,
> Mike
>
>> +
>> +/**
>> * devfreq_do() - Check the usage profile of a given device and configure
>> * frequency and voltage accordingly
>> * @devfreq: devfreq info of the given device
>> @@ -149,6 +167,8 @@ static void devfreq_monitor(struct work_struct *work)
>> dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
>> error, devfreq->governor->name);
>>
>> + sysfs_unmerge_group(&devfreq->dev->kobj,
>> + &dev_attr_group);
>> list_del(&devfreq->node);
>> kfree(devfreq);
>>
>> @@ -239,6 +259,8 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
>> queue_delayed_work(devfreq_wq, &devfreq_work,
>> devfreq->next_polling);
>> }
>> +
>> + sysfs_merge_group(&dev->kobj, &dev_attr_group);
>> out:
>> mutex_unlock(&devfreq_list_lock);
>>
>> @@ -271,6 +293,8 @@ int devfreq_remove_device(struct device *dev)
>> goto out;
>> }
>>
>> + sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
>> +
>> list_del(&devfreq->node);
>> srcu_notifier_chain_unregister(nh, &devfreq->nb);
>> kfree(devfreq);
>> @@ -279,6 +303,132 @@ out:
>> return 0;
>> }
>>
>> +static ssize_t show_governor(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->governor)
>> + return -EINVAL;
>> +
>> + return sprintf(buf, "%s\n", df->governor->name);
>> +}
>> +
>> +static ssize_t show_freq(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> +
>> + return sprintf(buf, "%lu\n", df->previous_freq);
>> +}
>> +
>> +static ssize_t show_max_freq(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> + unsigned long freq = ULONG_MAX;
>> + struct opp *opp;
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->dev)
>> + return -EINVAL;
>> +
>> + opp = opp_find_freq_floor(df->dev, &freq);
>> + if (IS_ERR(opp))
>> + return PTR_ERR(opp);
>> +
>> + return sprintf(buf, "%lu\n", freq);
>> +}
>> +
>> +static ssize_t show_min_freq(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> + unsigned long freq = 0;
>> + struct opp *opp;
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->dev)
>> + return -EINVAL;
>> +
>> + opp = opp_find_freq_ceil(df->dev, &freq);
>> + if (IS_ERR(opp))
>> + return PTR_ERR(opp);
>> +
>> + return sprintf(buf, "%lu\n", freq);
>> +}
>> +
>> +static ssize_t show_polling_interval(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->profile)
>> + return -EINVAL;
>> +
>> + return sprintf(buf, "%d\n", df->profile->polling_ms);
>> +}
>> +
>> +static ssize_t store_polling_interval(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct devfreq *df = get_devfreq(dev);
>> + unsigned int value;
>> + int ret;
>> +
>> + if (IS_ERR(df))
>> + return PTR_ERR(df);
>> + if (!df->profile)
>> + return -EINVAL;
>> +
>> + ret = sscanf(buf, "%u", &value);
>> + if (ret != 1)
>> + return -EINVAL;
>> +
>> + df->profile->polling_ms = value;
>> + df->next_polling = df->polling_jiffies
>> + = msecs_to_jiffies(value);
>> +
>> + mutex_lock(&devfreq_list_lock);
>> + if (df->next_polling > 0 && !polling) {
>> + polling = true;
>> + queue_delayed_work(devfreq_wq, &devfreq_work,
>> + df->next_polling);
>> + }
>> + mutex_unlock(&devfreq_list_lock);
>> +
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR(devfreq_governor, 0444, show_governor, NULL);
>> +static DEVICE_ATTR(devfreq_cur_freq, 0444, show_freq, NULL);
>> +static DEVICE_ATTR(devfreq_max_freq, 0444, show_max_freq, NULL);
>> +static DEVICE_ATTR(devfreq_min_freq, 0444, show_min_freq, NULL);
>> +static DEVICE_ATTR(devfreq_polling_interval, 0644, show_polling_interval,
>> + store_polling_interval);
>> +static struct attribute *dev_entries[] = {
>> + &dev_attr_devfreq_governor.attr,
>> + &dev_attr_devfreq_cur_freq.attr,
>> + &dev_attr_devfreq_max_freq.attr,
>> + &dev_attr_devfreq_min_freq.attr,
>> + &dev_attr_devfreq_polling_interval.attr,
>> + NULL,
>> +};
>> +static struct attribute_group dev_attr_group = {
>> + .name = power_group_name,
>> + .attrs = dev_entries,
>> +};
>> +
>> /**
>> * devfreq_init() - Initialize data structure for devfreq framework and
>> * start polling registered devfreq devices.
>> --
>> 1.7.4.1
>>
>>
>
--
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH v8 5/5] PM / DEVFREQ: add basic governors
From: MyungJoo Ham @ 2011-08-30 4:19 UTC (permalink / raw)
To: Turquette, Mike
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <CAJOA=zO6VUXEi1ymyi2sdv03VWgPFiq1Xus9V1-YHNjpnKRo2A@mail.gmail.com>
On Tue, Aug 30, 2011 at 3:58 AM, Turquette, Mike <mturquette@ti.com> wrote:
> On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> +What: /sys/devices/.../power/devfreq_userspace_set_freq
>
> How about just .../devfreq_set_freq? I think the userspace bit is
> implied and the name is very long.
>
Umm.. as this entry became userspace specific, I though I need it be
to distinguished from entries created by devfreq framework. However,
this one is created only while userspace is being used (device may
replace governors by calling remove and add); thus, there wouldn't be
any duplicated name issues. So, I think removing userspace from the
name should be fine. I will do so in the next revision.
>> +
>> +static ssize_t show_freq(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct devfreq *devfreq = get_devfreq(dev);
>> + struct userspace_data *data;
>> + int err = 0;
>> +
>> + if (IS_ERR(devfreq)) {
>> + err = PTR_ERR(devfreq);
>> + goto out;
>> + }
>> + data = devfreq->data;
>> +
>> + if (data->valid)
>> + err = sprintf(buf, "%lu\n", data->user_frequency);
>> + else
>> + err = sprintf(buf, "undefined\n");
>> +out:
>> + return err;
>> +}
>
> Shouldn't accesses to devfreq->data be protected by a mutex?
>
> Regards,
> Mike
No, it doesn't need mutex here. Although generally, a mutex will be
needed for simliar codes, in this specific case, devfreq->data does
not need a mutex protection.
There are two variables in data: "user_frequency" and "valid"
- valid is initially false and becomes true at store_freq() and never changes.
- store_freq() writes user_frequency and then writes valid as true.
(no one writes false on valid)
- user_frequency is read only when valid is true.
Thus, the case where mutex protection serializes with some dictinction is:
1. Init. (valid = false)
2. store_freq() writes user_frequency = X
3. show_freq()/devfreq_userspace_func() reads valid (false)
4. store_freq() writes valid = true
5. show_freq()/devfreq_userspace_func() returns without reading
devfreq_userspace_func()
6. store_freq() returns.
into
A.
1. Init (valid = false)
2. store_freq() starts and finishes
3. show_freq()/devfreq_userspace_func() starts and finishes
B.
1. Init (valid = false)
2. show_freq()/devfreq_userspace_func() starts and finishes
3. store_freq() starts and finishes
where B results in the same thing as non-mutex version does.
If there is an operation that makes valid false, we will need a mutex
(local to the governor) anyway.
Anyway, I agree with you on the point that governors might need a
locking mechanism in general; not just on the private data, but on the
devfreq access. I'll put more on this issue on the other thread.
Thank you.
Cheers,
MyungJoo
>
>> +
>> +static DEVICE_ATTR(devfreq_userspace_set_freq, 0644, show_freq, store_freq);
>> +static struct attribute *dev_entries[] = {
>> + &dev_attr_devfreq_userspace_set_freq.attr,
>> + NULL,
>> +};
>> +static struct attribute_group dev_attr_group = {
>> + .name = power_group_name,
>> + .attrs = dev_entries,
>> +};
>> +
>> +static int userspace_init(struct devfreq *devfreq)
>> +{
>> + int err = 0;
>> + struct userspace_data *data = kzalloc(sizeof(struct userspace_data),
>> + GFP_KERNEL);
>> +
>> + if (!data) {
>> + err = -ENOMEM;
>> + goto out;
>> + }
>> + data->valid = false;
>> + devfreq->data = data;
>> +
>> + sysfs_merge_group(&devfreq->dev->kobj, &dev_attr_group);
>> +out:
>> + return err;
>> +}
>> +
>> +static void userspace_exit(struct devfreq *devfreq)
>> +{
>> + sysfs_unmerge_group(&devfreq->dev->kobj, &dev_attr_group);
>> + kfree(devfreq->data);
>> + devfreq->data = NULL;
>> +}
>> +
>> +struct devfreq_governor devfreq_userspace = {
>> + .name = "userspace",
>> + .get_target_freq = devfreq_userspace_func,
>> + .init = userspace_init,
>> + .exit = userspace_exit,
>> +};
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index fdc6916..cbafcdf 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -13,6 +13,7 @@
>> #ifndef __LINUX_DEVFREQ_H__
>> #define __LINUX_DEVFREQ_H__
>>
>> +#include <linux/opp.h>
>> #include <linux/notifier.h>
>>
>> #define DEVFREQ_NAME_LEN 16
>> @@ -65,6 +66,8 @@ struct devfreq_governor {
>> * "devfreq_monitor" executions to reevaluate
>> * frequency/voltage of the device. Set by
>> * profile's polling_ms interval.
>> + * @user_set_freq User specified adequete frequency value (thru sysfs
>> + * interface). Governors may and may not use this value.
>> * @data Private data of the governor. The devfreq framework does not
>> * touch this.
>> *
>> @@ -82,6 +85,7 @@ struct devfreq {
>> unsigned long previous_freq;
>> unsigned int next_polling;
>>
>> + unsigned long user_set_freq; /* governors may ignore this. */
>> void *data; /* private data for governors */
>> };
>>
>> @@ -91,6 +95,37 @@ extern int devfreq_add_device(struct device *dev,
>> struct devfreq_governor *governor,
>> void *data);
>> extern int devfreq_remove_device(struct device *dev);
>> +
>> +#ifdef CONFIG_DEVFREQ_GOV_POWERSAVE
>> +extern struct devfreq_governor devfreq_powersave;
>> +#endif
>> +#ifdef CONFIG_DEVFREQ_GOV_PERFORMANCE
>> +extern struct devfreq_governor devfreq_performance;
>> +#endif
>> +#ifdef CONFIG_DEVFREQ_GOV_USERSPACE
>> +extern struct devfreq_governor devfreq_userspace;
>> +#endif
>> +#ifdef CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND
>> +extern struct devfreq_governor devfreq_simple_ondemand;
>> +/**
>> + * struct devfreq_simple_ondemand_data - void *data fed to struct devfreq
>> + * and devfreq_add_device
>> + * @ upthreshold If the load is over this value, the frequency jumps.
>> + * Specify 0 to use the default. Valid value = 0 to 100.
>> + * @ downdifferential If the load is under upthreshold - downdifferential,
>> + * the governor may consider slowing the frequency down.
>> + * Specify 0 to use the default. Valid value = 0 to 100.
>> + * downdifferential < upthreshold must hold.
>> + *
>> + * If the fed devfreq_simple_ondemand_data pointer is NULL to the governor,
>> + * the governor uses the default values.
>> + */
>> +struct devfreq_simple_ondemand_data {
>> + unsigned int upthreshold;
>> + unsigned int downdifferential;
>> +};
>> +#endif
>> +
>> #else /* !CONFIG_PM_DEVFREQ */
>> static int devfreq_add_device(struct device *dev,
>> struct devfreq_dev_profile *profile,
>> @@ -104,6 +139,12 @@ static int devfreq_remove_device(struct device *dev)
>> {
>> return 0;
>> }
>> +
>> +#define devfreq_powersave NULL
>> +#define devfreq_performance NULL
>> +#define devfreq_userspace NULL
>> +#define devfreq_simple_ondemand NULL
>> +
>> #endif /* CONFIG_PM_DEVFREQ */
>>
>> #endif /* __LINUX_DEVFREQ_H__ */
>> --
>> 1.7.4.1
>>
>>
>
--
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH v8 0/5] DEVFREQ, DVFS Framework for Non-CPU Devices.
From: Rafael J. Wysocki @ 2011-08-29 20:55 UTC (permalink / raw)
To: Turquette, Mike
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, MyungJoo Ham,
linux-pm, Thomas Gleixner
In-Reply-To: <CAJOA=zN_TqAz5tyXp3uRe4MezxLDAF5Dhzb6kst2xWi9zzr5dg@mail.gmail.com>
Hi,
On Monday, August 29, 2011, Turquette, Mike wrote:
> Rafael,
>
> Locking doesn't seem safe in v8; I replied with my comments.
>
> I should be free this week to review the next round a little more
> quickly.
Cool, thanks a lot for your hard work as a reviewer.
> Last week was hectic.
Sorry about that.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH v8 0/5] DEVFREQ, DVFS Framework for Non-CPU Devices.
From: Turquette, Mike @ 2011-08-29 19:22 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, MyungJoo Ham,
linux-pm, Thomas Gleixner
In-Reply-To: <201108272235.39334.rjw@sisk.pl>
Rafael,
Locking doesn't seem safe in v8; I replied with my comments.
I should be free this week to review the next round a little more
quickly. Last week was hectic.
Regards,
Mike
2011/8/27 Rafael J. Wysocki <rjw@sisk.pl>:
> Mike, are patches [3-5/5] in this revision fine by you?
>
> Rafael
>
>
> On Wednesday, August 24, 2011, MyungJoo Ham wrote:
>> The patchset revision v8 has minor updates since v7 and v6.
>> - Allow governors to have their own sysfs interface and init/exit callbacks.
>>
>> The patches 1/5 (OPP notifier) and 2/5 (DEVFREQ core) have no changes since v7.
>> There has been reordering between "add common sysfs interfaces" patch
>> and "add basic governors" (3/5 and 5/5)
>> "add internal interfaces for governors (4/5)" patch has been newly
>> introduced at v8 patchset.
>>
>> For a usage example, please look at
>> http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/devfreq
>>
>> In the above git tree, DVFS (dynamic voltage and frequency scaling) mechanism
>> is applied to the memory bus of Exynos4210 for Exynos4210-NURI boards.
>> In the example, the LPDDR2 DRAM frequency changes between 133, 266, and 400MHz
>> and other related clocks simply follow the determined DDR RAM clock.
>>
>> The DEVFREQ driver for Exynos4210 memory bus is at
>> /drivers/devfreq/exynos4210_memorybus.c in the git tree.
>>
>> In the dd (writing and reading 360MiB) test with NURI board, the memory
>> throughput was not changed (the performance is not deteriorated) while
>> the SoC power consumption has been reduced by 1%. When the memory access
>> is not that intense while the CPU is heavily used, the SoC power consumption
>> has been reduced by 6%. The power consumption has been compared with the
>> case using the conventional Exynos4210 CPUFREQ driver, which sets memory
>> bus frequency according to the CPU core frequency. Besides, when the CPU core
>> running slow and the memory access is intense, the performance (memory
>> throughput) has been increased by 11% (with higher SoC power consumption of
>> 5%). The tested governor is "simple-ondemand".
>>
>> MyungJoo Ham (5):
>> PM / OPP: Add OPP availability change notifier.
>> PM: Introduce DEVFREQ: generic DVFS framework with device-specific
>> OPPs
>> PM / DEVFREQ: add common sysfs interfaces
>> PM / DEVFREQ: add internal interfaces for governors
>> PM / DEVFREQ: add basic governors
>>
>> Documentation/ABI/testing/sysfs-devices-power | 46 +++
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 2 +
>> drivers/base/power/opp.c | 29 ++
>> drivers/devfreq/Kconfig | 75 ++++
>> drivers/devfreq/Makefile | 5 +
>> drivers/devfreq/devfreq.c | 463 +++++++++++++++++++++++++
>> drivers/devfreq/governor.h | 20 +
>> drivers/devfreq/governor_performance.c | 24 ++
>> drivers/devfreq/governor_powersave.c | 24 ++
>> drivers/devfreq/governor_simpleondemand.c | 88 +++++
>> drivers/devfreq/governor_userspace.c | 119 +++++++
>> include/linux/devfreq.h | 150 ++++++++
>> include/linux/opp.h | 12 +
>> 14 files changed, 1059 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/devfreq/Kconfig
>> create mode 100644 drivers/devfreq/Makefile
>> create mode 100644 drivers/devfreq/devfreq.c
>> create mode 100644 drivers/devfreq/governor.h
>> create mode 100644 drivers/devfreq/governor_performance.c
>> create mode 100644 drivers/devfreq/governor_powersave.c
>> create mode 100644 drivers/devfreq/governor_simpleondemand.c
>> create mode 100644 drivers/devfreq/governor_userspace.c
>> create mode 100644 include/linux/devfreq.h
>>
>>
>
>
^ permalink raw reply
* Re: [PATCH v8 4/5] PM / DEVFREQ: add internal interfaces for governors
From: Turquette, Mike @ 2011-08-29 19:21 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <1314174131-14194-5-git-send-email-myungjoo.ham@samsung.com>
On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> - get_devfreq(): governors may convert struct dev -> struct devfreq
> - update_devfreq(): governors may notify DEVFREQ core to reevaluate
> frequencies.
> - Governors may have .init and .exit callbacks
>
> In order to use the internal interface, get_devfreq() and
> update_devfreq(), governor should include "governor.h"
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Please see patch 3 and 5 for my thoughts on locking in struct devfreq.
Also, this patch doesn't need to be separate, but can be rolled into
patch 3.
Regards,
Mike
> ---
> drivers/devfreq/devfreq.c | 32 ++++++++++++++++++++++++--------
> drivers/devfreq/governor.h | 20 ++++++++++++++++++++
> include/linux/devfreq.h | 2 ++
> 3 files changed, 46 insertions(+), 8 deletions(-)
> create mode 100644 drivers/devfreq/governor.h
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 5bbece6..8de3b21 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -65,10 +65,10 @@ static struct devfreq *find_device_devfreq(struct device *dev)
>
> /**
> * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq()
> - * with mutex protection.
> + * with mutex protection. exported for governors
> * @dev: device pointer used to lookup device devfreq.
> */
> -static struct devfreq *get_devfreq(struct device *dev)
> +struct devfreq *get_devfreq(struct device *dev)
> {
> struct devfreq *ret;
>
> @@ -113,17 +113,15 @@ static int devfreq_do(struct devfreq *devfreq)
> }
>
> /**
> - * devfreq_update() - Notify that the device OPP has been changed.
> - * @dev: the device whose OPP has been changed.
> + * update_devfreq() - Notify that the device OPP or frequency requirement
> + * has been changed. This function is exported for governors.
> + * @devfreq: the devfreq instance.
> */
> -static int devfreq_update(struct notifier_block *nb, unsigned long type,
> - void *devp)
> +int update_devfreq(struct devfreq *devfreq)
> {
> - struct devfreq *devfreq;
> int err = 0;
>
> mutex_lock(&devfreq_list_lock);
> - devfreq = container_of(nb, struct devfreq, nb);
> /* Reevaluate the proper frequency */
> err = devfreq_do(devfreq);
> mutex_unlock(&devfreq_list_lock);
> @@ -131,6 +129,18 @@ static int devfreq_update(struct notifier_block *nb, unsigned long type,
> }
>
> /**
> + * devfreq_update() - Notify that the device OPP has been changed.
> + * @dev: the device whose OPP has been changed.
> + */
> +static int devfreq_update(struct notifier_block *nb, unsigned long type,
> + void *devp)
> +{
> + struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
> +
> + return update_devfreq(devfreq);
> +}
> +
> +/**
> * devfreq_monitor() - Periodically run devfreq_do()
> * @work: the work struct used to run devfreq_monitor periodically.
> *
> @@ -254,6 +264,9 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
>
> list_add(&devfreq->node, &devfreq_list);
>
> + if (governor->init)
> + governor->init(devfreq);
> +
> if (devfreq_wq && devfreq->next_polling && !polling) {
> polling = true;
> queue_delayed_work(devfreq_wq, &devfreq_work,
> @@ -295,6 +308,9 @@ int devfreq_remove_device(struct device *dev)
>
> sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
>
> + if (devfreq->governor->exit)
> + devfreq->governor->exit(devfreq);
> +
> list_del(&devfreq->node);
> srcu_notifier_chain_unregister(nh, &devfreq->nb);
> kfree(devfreq);
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> new file mode 100644
> index 0000000..bb5d964
> --- /dev/null
> +++ b/drivers/devfreq/governor.h
> @@ -0,0 +1,20 @@
> +/*
> + * governor.h - internal header for governors.
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * MyungJoo Ham <myungjoo.ham@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This header is for devfreq governors in drivers/devfreq/
> + */
> +
> +#ifndef _GOVERNOR_H
> +#define _GOVERNOR_H
> +
> +extern struct devfreq *get_devfreq(struct device *dev);
> +extern int update_devfreq(struct devfreq *devfreq);
> +
> +#endif /* _GOVERNOR_H */
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 8252238..fdc6916 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -46,6 +46,8 @@ struct devfreq_dev_profile {
> struct devfreq_governor {
> char name[DEVFREQ_NAME_LEN];
> int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
> + int (*init)(struct devfreq *this);
> + void (*exit)(struct devfreq *this);
> };
>
> /**
> --
> 1.7.4.1
>
>
^ permalink raw reply
* Re: [PATCH v8 3/5] PM / DEVFREQ: add common sysfs interfaces
From: Turquette, Mike @ 2011-08-29 19:17 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <1314174131-14194-4-git-send-email-myungjoo.ham@samsung.com>
On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> Device specific sysfs interface /sys/devices/.../power/devfreq_*
> - governor R: name of governor
> - cur_freq R: current frequency
> - max_freq R: maximum operable frequency
> - min_freq R: minimum operable frequency
> - polling_interval R: polling interval in ms given with devfreq profile
> W: update polling interval.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> --
> Changed from v7
> - removed set_freq from the common devfreq interface
> - added get_devfreq, a mutex-protected wrapper for find_device_devfreq
> (for sysfs interfaces and later with governor-support)
> - corrected ABI documentation.
>
> Changed from v6
> - poling_interval is writable.
>
> Changed from v5
> - updated devferq_update usage.
>
> Changed from v4
> - removed system-wide sysfs interface
> - removed tickling sysfs interface
> - added set_freq for userspace governor (and any other governors that
> require user input)
>
> Changed from v3
> - corrected sysfs API usage
> - corrected error messages
> - moved sysfs entry location
> - added sysfs entries
>
> Changed from v2
> - add ABI entries for devfreq sysfs interface
> ---
> Documentation/ABI/testing/sysfs-devices-power | 37 ++++++
> drivers/devfreq/devfreq.c | 150 +++++++++++++++++++++++++
> 2 files changed, 187 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
> index 8ffbc25..57f4591 100644
> --- a/Documentation/ABI/testing/sysfs-devices-power
> +++ b/Documentation/ABI/testing/sysfs-devices-power
> @@ -165,3 +165,40 @@ Description:
>
> Not all drivers support this attribute. If it isn't supported,
> attempts to read or write it will yield I/O errors.
> +
> +What: /sys/devices/.../power/devfreq_governor
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_governor shows the name
> + of the governor used by the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_cur_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_cur_freq shows the current
> + frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_max_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_max_freq shows the
> + maximum operable frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_min_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_min_freq shows the
> + minimum operable frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_polling_interval
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_polling_interval sets and
> + shows the requested polling interval of the corresponding
> + device. The values are represented in ms. If the value is less
> + than 1 jiffy, it is considered to be 0, which means no polling.
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index df63bdc..5bbece6 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -37,6 +37,8 @@ static struct delayed_work devfreq_work;
> static LIST_HEAD(devfreq_list);
> static DEFINE_MUTEX(devfreq_list_lock);
>
> +static struct attribute_group dev_attr_group;
> +
> /**
> * find_device_devfreq() - find devfreq struct using device pointer
> * @dev: device pointer used to lookup device devfreq.
> @@ -62,6 +64,22 @@ static struct devfreq *find_device_devfreq(struct device *dev)
> }
>
> /**
> + * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq()
> + * with mutex protection.
> + * @dev: device pointer used to lookup device devfreq.
> + */
> +static struct devfreq *get_devfreq(struct device *dev)
> +{
> + struct devfreq *ret;
> +
> + mutex_lock(&devfreq_list_lock);
> + ret = find_device_devfreq(dev);
> + mutex_unlock(&devfreq_list_lock);
> +
> + return ret;
> +}
Sorry I'll have to rescind my ACK. I just reviewed patch 5/5 and the
show_freq and restore_freq functions for the userspace governor do not
protect the private data accesses with a mutex. Looking a bit further
I see that they do call get_devfreq; I think the semantics for this
are wrong.
get_devfreq only protects the list as long as it takes to do a lookup.
However neither struct devfreq or struct userspace_data have a mutex
associated with them, so concurrent operations are not protected.
One heavy-handed way of solving this is to not have get_devfreq
release the mutex, and then have those functions call a new
put_devfreq which does release the mutex. However that is really bad
locking.
What do you think about putting a mutex in struct devfreq? The rule
for governors can be that access to the governor-specific private data
requires taking that devfreq's mutex.
Regards,
Mike
> +
> +/**
> * devfreq_do() - Check the usage profile of a given device and configure
> * frequency and voltage accordingly
> * @devfreq: devfreq info of the given device
> @@ -149,6 +167,8 @@ static void devfreq_monitor(struct work_struct *work)
> dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
> error, devfreq->governor->name);
>
> + sysfs_unmerge_group(&devfreq->dev->kobj,
> + &dev_attr_group);
> list_del(&devfreq->node);
> kfree(devfreq);
>
> @@ -239,6 +259,8 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
> queue_delayed_work(devfreq_wq, &devfreq_work,
> devfreq->next_polling);
> }
> +
> + sysfs_merge_group(&dev->kobj, &dev_attr_group);
> out:
> mutex_unlock(&devfreq_list_lock);
>
> @@ -271,6 +293,8 @@ int devfreq_remove_device(struct device *dev)
> goto out;
> }
>
> + sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
> +
> list_del(&devfreq->node);
> srcu_notifier_chain_unregister(nh, &devfreq->nb);
> kfree(devfreq);
> @@ -279,6 +303,132 @@ out:
> return 0;
> }
>
> +static ssize_t show_governor(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->governor)
> + return -EINVAL;
> +
> + return sprintf(buf, "%s\n", df->governor->name);
> +}
> +
> +static ssize_t show_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> +
> + return sprintf(buf, "%lu\n", df->previous_freq);
> +}
> +
> +static ssize_t show_max_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned long freq = ULONG_MAX;
> + struct opp *opp;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->dev)
> + return -EINVAL;
> +
> + opp = opp_find_freq_floor(df->dev, &freq);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + return sprintf(buf, "%lu\n", freq);
> +}
> +
> +static ssize_t show_min_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned long freq = 0;
> + struct opp *opp;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->dev)
> + return -EINVAL;
> +
> + opp = opp_find_freq_ceil(df->dev, &freq);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + return sprintf(buf, "%lu\n", freq);
> +}
> +
> +static ssize_t show_polling_interval(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->profile)
> + return -EINVAL;
> +
> + return sprintf(buf, "%d\n", df->profile->polling_ms);
> +}
> +
> +static ssize_t store_polling_interval(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned int value;
> + int ret;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->profile)
> + return -EINVAL;
> +
> + ret = sscanf(buf, "%u", &value);
> + if (ret != 1)
> + return -EINVAL;
> +
> + df->profile->polling_ms = value;
> + df->next_polling = df->polling_jiffies
> + = msecs_to_jiffies(value);
> +
> + mutex_lock(&devfreq_list_lock);
> + if (df->next_polling > 0 && !polling) {
> + polling = true;
> + queue_delayed_work(devfreq_wq, &devfreq_work,
> + df->next_polling);
> + }
> + mutex_unlock(&devfreq_list_lock);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(devfreq_governor, 0444, show_governor, NULL);
> +static DEVICE_ATTR(devfreq_cur_freq, 0444, show_freq, NULL);
> +static DEVICE_ATTR(devfreq_max_freq, 0444, show_max_freq, NULL);
> +static DEVICE_ATTR(devfreq_min_freq, 0444, show_min_freq, NULL);
> +static DEVICE_ATTR(devfreq_polling_interval, 0644, show_polling_interval,
> + store_polling_interval);
> +static struct attribute *dev_entries[] = {
> + &dev_attr_devfreq_governor.attr,
> + &dev_attr_devfreq_cur_freq.attr,
> + &dev_attr_devfreq_max_freq.attr,
> + &dev_attr_devfreq_min_freq.attr,
> + &dev_attr_devfreq_polling_interval.attr,
> + NULL,
> +};
> +static struct attribute_group dev_attr_group = {
> + .name = power_group_name,
> + .attrs = dev_entries,
> +};
> +
> /**
> * devfreq_init() - Initialize data structure for devfreq framework and
> * start polling registered devfreq devices.
> --
> 1.7.4.1
>
>
^ permalink raw reply
* Re: [PATCH v8 5/5] PM / DEVFREQ: add basic governors
From: Turquette, Mike @ 2011-08-29 18:58 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <1314174131-14194-6-git-send-email-myungjoo.ham@samsung.com>
On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> Four CPUFREQ-like governors are provided as examples.
>
> powersave: use the lowest frequency possible. The user (device) should
> set the polling_ms as 0 because polling is useless for this governor.
>
> performance: use the highest freqeuncy possible. The user (device)
> should set the polling_ms as 0 because polling is useless for this
> governor.
>
> userspace: use the user specified frequency stored at
> devfreq.user_set_freq. With sysfs support in the following patch, a user
> may set the value with the sysfs interface.
>
> simple_ondemand: simplified version of CPUFREQ's ONDEMAND governor.
>
> When a user updates OPP entries (enable/disable/add), OPP framework
> automatically notifies DEVFREQ to update operating frequency
> accordingly. Thus, DEVFREQ users (device drivers) do not need to update
> DEVFREQ manually with OPP entry updates or set polling_ms for powersave
> , performance, userspace, or any other "static" governors.
>
> Note that these are given only as basic examples for governors and any
> devices with DEVFREQ may implement their own governors with the drivers
> and use them.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> ---
> Changed from v7
> - Userspace uses its own sysfs interface.
>
> Changed from v5
> - Seperated governor files from devfreq.c
> - Allow simple ondemand to be tuned for each device
> ---
> Documentation/ABI/testing/sysfs-devices-power | 9 ++
> drivers/devfreq/Kconfig | 36 ++++++++
> drivers/devfreq/Makefile | 4 +
> drivers/devfreq/governor_performance.c | 24 +++++
> drivers/devfreq/governor_powersave.c | 24 +++++
> drivers/devfreq/governor_simpleondemand.c | 88 ++++++++++++++++++
> drivers/devfreq/governor_userspace.c | 119 +++++++++++++++++++++++++
> include/linux/devfreq.h | 41 +++++++++
> 8 files changed, 345 insertions(+), 0 deletions(-)
> create mode 100644 drivers/devfreq/governor_performance.c
> create mode 100644 drivers/devfreq/governor_powersave.c
> create mode 100644 drivers/devfreq/governor_simpleondemand.c
> create mode 100644 drivers/devfreq/governor_userspace.c
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
> index 57f4591..c7f6977 100644
> --- a/Documentation/ABI/testing/sysfs-devices-power
> +++ b/Documentation/ABI/testing/sysfs-devices-power
> @@ -202,3 +202,12 @@ Description:
> shows the requested polling interval of the corresponding
> device. The values are represented in ms. If the value is less
> than 1 jiffy, it is considered to be 0, which means no polling.
> +
> +What: /sys/devices/.../power/devfreq_userspace_set_freq
How about just .../devfreq_set_freq? I think the userspace bit is
implied and the name is very long.
> +Date: August 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_userspace_set_freq sets
> + and shows the user specified frequency in kHz. This sysfs
> + entry is created and managed by userspace DEVFREQ governor.
> + If other governors are used, it won't be supported.
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 1fb42de..643b055 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -34,6 +34,42 @@ menuconfig PM_DEVFREQ
>
> if PM_DEVFREQ
>
> +comment "DEVFREQ Governors"
> +
> +config DEVFREQ_GOV_SIMPLE_ONDEMAND
> + bool "Simple Ondemand"
> + help
> + Chooses frequency based on the recent load on the device. Works
> + similar as ONDEMAND governor of CPUFREQ does. A device with
> + Simple-Ondemand should be able to provide busy/total counter
> + values that imply the usage rate. A device may provide tuned
> + values to the governor with data field at devfreq_add_device().
> +
> +config DEVFREQ_GOV_PERFORMANCE
> + bool "Performance"
> + help
> + Sets the frequency at the maximum available frequency.
> + This governor always returns UINT_MAX as frequency so that
> + the DEVFREQ framework returns the highest frequency available
> + at any time.
> +
> +config DEVFREQ_GOV_POWERSAVE
> + bool "Powersave"
> + help
> + Sets the frequency at the minimum available frequency.
> + This governor always returns 0 as frequency so that
> + the DEVFREQ framework returns the lowest frequency available
> + at any time.
> +
> +config DEVFREQ_GOV_USERSPACE
> + bool "Userspace"
> + help
> + Sets the frequency at the user specified one.
> + This governor returns the user configured frequency if there
> + has been an input to /sys/devices/.../power/devfreq_set_freq.
> + Otherwise, the governor does not change the frequnecy
> + given at the initialization.
> +
> comment "DEVFREQ Drivers"
>
> endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 168934a..4564a89 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -1 +1,5 @@
> obj-$(CONFIG_PM_DEVFREQ) += devfreq.o
> +obj-$(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND) += governor_simpleondemand.o
> +obj-$(CONFIG_DEVFREQ_GOV_PERFORMANCE) += governor_performance.o
> +obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
> +obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
> diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c
> new file mode 100644
> index 0000000..c47eff8
> --- /dev/null
> +++ b/drivers/devfreq/governor_performance.c
> @@ -0,0 +1,24 @@
> +/*
> + * linux/drivers/devfreq/governor_performance.c
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * MyungJoo Ham <myungjoo.ham@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/devfreq.h>
> +
> +static int devfreq_performance_func(struct devfreq *df,
> + unsigned long *freq)
> +{
> + *freq = UINT_MAX; /* devfreq_do will run "floor" */
> + return 0;
> +}
> +
> +struct devfreq_governor devfreq_performance = {
> + .name = "performance",
> + .get_target_freq = devfreq_performance_func,
> +};
> diff --git a/drivers/devfreq/governor_powersave.c b/drivers/devfreq/governor_powersave.c
> new file mode 100644
> index 0000000..4f128d8
> --- /dev/null
> +++ b/drivers/devfreq/governor_powersave.c
> @@ -0,0 +1,24 @@
> +/*
> + * linux/drivers/devfreq/governor_powersave.c
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * MyungJoo Ham <myungjoo.ham@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/devfreq.h>
> +
> +static int devfreq_powersave_func(struct devfreq *df,
> + unsigned long *freq)
> +{
> + *freq = 0; /* devfreq_do will run "ceiling" to 0 */
> + return 0;
> +}
> +
> +struct devfreq_governor devfreq_powersave = {
> + .name = "powersave",
> + .get_target_freq = devfreq_powersave_func,
> +};
> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
> new file mode 100644
> index 0000000..18fe8be
> --- /dev/null
> +++ b/drivers/devfreq/governor_simpleondemand.c
> @@ -0,0 +1,88 @@
> +/*
> + * linux/drivers/devfreq/governor_simpleondemand.c
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * MyungJoo Ham <myungjoo.ham@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/devfreq.h>
> +#include <linux/math64.h>
> +
> +/* Default constants for DevFreq-Simple-Ondemand (DFSO) */
> +#define DFSO_UPTHRESHOLD (90)
> +#define DFSO_DOWNDIFFERENCTIAL (5)
> +static int devfreq_simple_ondemand_func(struct devfreq *df,
> + unsigned long *freq)
> +{
> + struct devfreq_dev_status stat;
> + int err = df->profile->get_dev_status(df->dev, &stat);
> + unsigned long long a, b;
> + unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD;
> + unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL;
> + struct devfreq_simple_ondemand_data *data = df->data;
> +
> + if (err)
> + return err;
> +
> + if (data) {
> + if (data->upthreshold)
> + dfso_upthreshold = data->upthreshold;
> + if (data->downdifferential)
> + dfso_downdifferential = data->downdifferential;
> + }
> + if (dfso_upthreshold > 100 ||
> + dfso_upthreshold < dfso_downdifferential)
> + return -EINVAL;
> +
> + /* Assume MAX if it is going to be divided by zero */
> + if (stat.total_time == 0) {
> + *freq = UINT_MAX;
> + return 0;
> + }
> +
> + /* Prevent overflow */
> + if (stat.busy_time >= (1 << 24) || stat.total_time >= (1 << 24)) {
> + stat.busy_time >>= 7;
> + stat.total_time >>= 7;
> + }
> +
> + /* Set MAX if it's busy enough */
> + if (stat.busy_time * 100 >
> + stat.total_time * dfso_upthreshold) {
> + *freq = UINT_MAX;
> + return 0;
> + }
> +
> + /* Set MAX if we do not know the initial frequency */
> + if (stat.current_frequency == 0) {
> + *freq = UINT_MAX;
> + return 0;
> + }
> +
> + /* Keep the current frequency */
> + if (stat.busy_time * 100 >
> + stat.total_time * (dfso_upthreshold - dfso_downdifferential)) {
> + *freq = stat.current_frequency;
> + return 0;
> + }
> +
> + /* Set the desired frequency based on the load */
> + a = stat.busy_time;
> + a *= stat.current_frequency;
> + b = div_u64(a, stat.total_time);
> + b *= 100;
> + b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
> + *freq = (unsigned long) b;
> +
> + return 0;
> +}
> +
> +struct devfreq_governor devfreq_simple_ondemand = {
> + .name = "simple_ondemand",
> + .get_target_freq = devfreq_simple_ondemand_func,
> +};
> diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
> new file mode 100644
> index 0000000..53a4574
> --- /dev/null
> +++ b/drivers/devfreq/governor_userspace.c
> @@ -0,0 +1,119 @@
> +/*
> + * linux/drivers/devfreq/governor_simpleondemand.c
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * MyungJoo Ham <myungjoo.ham@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/devfreq.h>
> +#include <linux/pm.h>
> +#include "governor.h"
> +
> +struct userspace_data {
> + unsigned long user_frequency;
> + bool valid;
> +};
> +
> +static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq)
> +{
> + struct userspace_data *data = df->data;
> +
> + if (!data->valid)
> + *freq = df->previous_freq; /* No user freq specified yet */
> + else
> + *freq = data->user_frequency;
> + return 0;
> +}
> +
> +static ssize_t store_freq(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct devfreq *devfreq = get_devfreq(dev);
> + struct userspace_data *data;
> + unsigned long wanted;
> + int err = 0;
> +
> + if (IS_ERR(devfreq)) {
> + err = PTR_ERR(devfreq);
> + goto out;
> + }
> + data = devfreq->data;
> +
> + sscanf(buf, "%lu", &wanted);
> + data->user_frequency = wanted;
> + data->valid = true;
> + err = update_devfreq(devfreq);
> + if (err == 0)
> + err = count;
> +out:
> + return err;
> +}
> +
> +static ssize_t show_freq(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct devfreq *devfreq = get_devfreq(dev);
> + struct userspace_data *data;
> + int err = 0;
> +
> + if (IS_ERR(devfreq)) {
> + err = PTR_ERR(devfreq);
> + goto out;
> + }
> + data = devfreq->data;
> +
> + if (data->valid)
> + err = sprintf(buf, "%lu\n", data->user_frequency);
> + else
> + err = sprintf(buf, "undefined\n");
> +out:
> + return err;
> +}
Shouldn't accesses to devfreq->data be protected by a mutex?
Regards,
Mike
> +
> +static DEVICE_ATTR(devfreq_userspace_set_freq, 0644, show_freq, store_freq);
> +static struct attribute *dev_entries[] = {
> + &dev_attr_devfreq_userspace_set_freq.attr,
> + NULL,
> +};
> +static struct attribute_group dev_attr_group = {
> + .name = power_group_name,
> + .attrs = dev_entries,
> +};
> +
> +static int userspace_init(struct devfreq *devfreq)
> +{
> + int err = 0;
> + struct userspace_data *data = kzalloc(sizeof(struct userspace_data),
> + GFP_KERNEL);
> +
> + if (!data) {
> + err = -ENOMEM;
> + goto out;
> + }
> + data->valid = false;
> + devfreq->data = data;
> +
> + sysfs_merge_group(&devfreq->dev->kobj, &dev_attr_group);
> +out:
> + return err;
> +}
> +
> +static void userspace_exit(struct devfreq *devfreq)
> +{
> + sysfs_unmerge_group(&devfreq->dev->kobj, &dev_attr_group);
> + kfree(devfreq->data);
> + devfreq->data = NULL;
> +}
> +
> +struct devfreq_governor devfreq_userspace = {
> + .name = "userspace",
> + .get_target_freq = devfreq_userspace_func,
> + .init = userspace_init,
> + .exit = userspace_exit,
> +};
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index fdc6916..cbafcdf 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -13,6 +13,7 @@
> #ifndef __LINUX_DEVFREQ_H__
> #define __LINUX_DEVFREQ_H__
>
> +#include <linux/opp.h>
> #include <linux/notifier.h>
>
> #define DEVFREQ_NAME_LEN 16
> @@ -65,6 +66,8 @@ struct devfreq_governor {
> * "devfreq_monitor" executions to reevaluate
> * frequency/voltage of the device. Set by
> * profile's polling_ms interval.
> + * @user_set_freq User specified adequete frequency value (thru sysfs
> + * interface). Governors may and may not use this value.
> * @data Private data of the governor. The devfreq framework does not
> * touch this.
> *
> @@ -82,6 +85,7 @@ struct devfreq {
> unsigned long previous_freq;
> unsigned int next_polling;
>
> + unsigned long user_set_freq; /* governors may ignore this. */
> void *data; /* private data for governors */
> };
>
> @@ -91,6 +95,37 @@ extern int devfreq_add_device(struct device *dev,
> struct devfreq_governor *governor,
> void *data);
> extern int devfreq_remove_device(struct device *dev);
> +
> +#ifdef CONFIG_DEVFREQ_GOV_POWERSAVE
> +extern struct devfreq_governor devfreq_powersave;
> +#endif
> +#ifdef CONFIG_DEVFREQ_GOV_PERFORMANCE
> +extern struct devfreq_governor devfreq_performance;
> +#endif
> +#ifdef CONFIG_DEVFREQ_GOV_USERSPACE
> +extern struct devfreq_governor devfreq_userspace;
> +#endif
> +#ifdef CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND
> +extern struct devfreq_governor devfreq_simple_ondemand;
> +/**
> + * struct devfreq_simple_ondemand_data - void *data fed to struct devfreq
> + * and devfreq_add_device
> + * @ upthreshold If the load is over this value, the frequency jumps.
> + * Specify 0 to use the default. Valid value = 0 to 100.
> + * @ downdifferential If the load is under upthreshold - downdifferential,
> + * the governor may consider slowing the frequency down.
> + * Specify 0 to use the default. Valid value = 0 to 100.
> + * downdifferential < upthreshold must hold.
> + *
> + * If the fed devfreq_simple_ondemand_data pointer is NULL to the governor,
> + * the governor uses the default values.
> + */
> +struct devfreq_simple_ondemand_data {
> + unsigned int upthreshold;
> + unsigned int downdifferential;
> +};
> +#endif
> +
> #else /* !CONFIG_PM_DEVFREQ */
> static int devfreq_add_device(struct device *dev,
> struct devfreq_dev_profile *profile,
> @@ -104,6 +139,12 @@ static int devfreq_remove_device(struct device *dev)
> {
> return 0;
> }
> +
> +#define devfreq_powersave NULL
> +#define devfreq_performance NULL
> +#define devfreq_userspace NULL
> +#define devfreq_simple_ondemand NULL
> +
> #endif /* CONFIG_PM_DEVFREQ */
>
> #endif /* __LINUX_DEVFREQ_H__ */
> --
> 1.7.4.1
>
>
^ permalink raw reply
* Re: [PATCH v8 3/5] PM / DEVFREQ: add common sysfs interfaces
From: Turquette, Mike @ 2011-08-29 18:49 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm,
Thomas Gleixner
In-Reply-To: <1314174131-14194-4-git-send-email-myungjoo.ham@samsung.com>
On Wed, Aug 24, 2011 at 1:22 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> Device specific sysfs interface /sys/devices/.../power/devfreq_*
> - governor R: name of governor
> - cur_freq R: current frequency
> - max_freq R: maximum operable frequency
> - min_freq R: minimum operable frequency
> - polling_interval R: polling interval in ms given with devfreq profile
> W: update polling interval.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Looks good to me.
Reviewed-by: Mike Turquette <mturquette@ti.com>
Regards,
Mike
>
> --
> Changed from v7
> - removed set_freq from the common devfreq interface
> - added get_devfreq, a mutex-protected wrapper for find_device_devfreq
> (for sysfs interfaces and later with governor-support)
> - corrected ABI documentation.
>
> Changed from v6
> - poling_interval is writable.
>
> Changed from v5
> - updated devferq_update usage.
>
> Changed from v4
> - removed system-wide sysfs interface
> - removed tickling sysfs interface
> - added set_freq for userspace governor (and any other governors that
> require user input)
>
> Changed from v3
> - corrected sysfs API usage
> - corrected error messages
> - moved sysfs entry location
> - added sysfs entries
>
> Changed from v2
> - add ABI entries for devfreq sysfs interface
> ---
> Documentation/ABI/testing/sysfs-devices-power | 37 ++++++
> drivers/devfreq/devfreq.c | 150 +++++++++++++++++++++++++
> 2 files changed, 187 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
> index 8ffbc25..57f4591 100644
> --- a/Documentation/ABI/testing/sysfs-devices-power
> +++ b/Documentation/ABI/testing/sysfs-devices-power
> @@ -165,3 +165,40 @@ Description:
>
> Not all drivers support this attribute. If it isn't supported,
> attempts to read or write it will yield I/O errors.
> +
> +What: /sys/devices/.../power/devfreq_governor
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_governor shows the name
> + of the governor used by the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_cur_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_cur_freq shows the current
> + frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_max_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_max_freq shows the
> + maximum operable frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_min_freq
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_min_freq shows the
> + minimum operable frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_polling_interval
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_polling_interval sets and
> + shows the requested polling interval of the corresponding
> + device. The values are represented in ms. If the value is less
> + than 1 jiffy, it is considered to be 0, which means no polling.
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index df63bdc..5bbece6 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -37,6 +37,8 @@ static struct delayed_work devfreq_work;
> static LIST_HEAD(devfreq_list);
> static DEFINE_MUTEX(devfreq_list_lock);
>
> +static struct attribute_group dev_attr_group;
> +
> /**
> * find_device_devfreq() - find devfreq struct using device pointer
> * @dev: device pointer used to lookup device devfreq.
> @@ -62,6 +64,22 @@ static struct devfreq *find_device_devfreq(struct device *dev)
> }
>
> /**
> + * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq()
> + * with mutex protection.
> + * @dev: device pointer used to lookup device devfreq.
> + */
> +static struct devfreq *get_devfreq(struct device *dev)
> +{
> + struct devfreq *ret;
> +
> + mutex_lock(&devfreq_list_lock);
> + ret = find_device_devfreq(dev);
> + mutex_unlock(&devfreq_list_lock);
> +
> + return ret;
> +}
> +
> +/**
> * devfreq_do() - Check the usage profile of a given device and configure
> * frequency and voltage accordingly
> * @devfreq: devfreq info of the given device
> @@ -149,6 +167,8 @@ static void devfreq_monitor(struct work_struct *work)
> dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
> error, devfreq->governor->name);
>
> + sysfs_unmerge_group(&devfreq->dev->kobj,
> + &dev_attr_group);
> list_del(&devfreq->node);
> kfree(devfreq);
>
> @@ -239,6 +259,8 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
> queue_delayed_work(devfreq_wq, &devfreq_work,
> devfreq->next_polling);
> }
> +
> + sysfs_merge_group(&dev->kobj, &dev_attr_group);
> out:
> mutex_unlock(&devfreq_list_lock);
>
> @@ -271,6 +293,8 @@ int devfreq_remove_device(struct device *dev)
> goto out;
> }
>
> + sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
> +
> list_del(&devfreq->node);
> srcu_notifier_chain_unregister(nh, &devfreq->nb);
> kfree(devfreq);
> @@ -279,6 +303,132 @@ out:
> return 0;
> }
>
> +static ssize_t show_governor(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->governor)
> + return -EINVAL;
> +
> + return sprintf(buf, "%s\n", df->governor->name);
> +}
> +
> +static ssize_t show_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> +
> + return sprintf(buf, "%lu\n", df->previous_freq);
> +}
> +
> +static ssize_t show_max_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned long freq = ULONG_MAX;
> + struct opp *opp;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->dev)
> + return -EINVAL;
> +
> + opp = opp_find_freq_floor(df->dev, &freq);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + return sprintf(buf, "%lu\n", freq);
> +}
> +
> +static ssize_t show_min_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned long freq = 0;
> + struct opp *opp;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->dev)
> + return -EINVAL;
> +
> + opp = opp_find_freq_ceil(df->dev, &freq);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + return sprintf(buf, "%lu\n", freq);
> +}
> +
> +static ssize_t show_polling_interval(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df = get_devfreq(dev);
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->profile)
> + return -EINVAL;
> +
> + return sprintf(buf, "%d\n", df->profile->polling_ms);
> +}
> +
> +static ssize_t store_polling_interval(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct devfreq *df = get_devfreq(dev);
> + unsigned int value;
> + int ret;
> +
> + if (IS_ERR(df))
> + return PTR_ERR(df);
> + if (!df->profile)
> + return -EINVAL;
> +
> + ret = sscanf(buf, "%u", &value);
> + if (ret != 1)
> + return -EINVAL;
> +
> + df->profile->polling_ms = value;
> + df->next_polling = df->polling_jiffies
> + = msecs_to_jiffies(value);
> +
> + mutex_lock(&devfreq_list_lock);
> + if (df->next_polling > 0 && !polling) {
> + polling = true;
> + queue_delayed_work(devfreq_wq, &devfreq_work,
> + df->next_polling);
> + }
> + mutex_unlock(&devfreq_list_lock);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(devfreq_governor, 0444, show_governor, NULL);
> +static DEVICE_ATTR(devfreq_cur_freq, 0444, show_freq, NULL);
> +static DEVICE_ATTR(devfreq_max_freq, 0444, show_max_freq, NULL);
> +static DEVICE_ATTR(devfreq_min_freq, 0444, show_min_freq, NULL);
> +static DEVICE_ATTR(devfreq_polling_interval, 0644, show_polling_interval,
> + store_polling_interval);
> +static struct attribute *dev_entries[] = {
> + &dev_attr_devfreq_governor.attr,
> + &dev_attr_devfreq_cur_freq.attr,
> + &dev_attr_devfreq_max_freq.attr,
> + &dev_attr_devfreq_min_freq.attr,
> + &dev_attr_devfreq_polling_interval.attr,
> + NULL,
> +};
> +static struct attribute_group dev_attr_group = {
> + .name = power_group_name,
> + .attrs = dev_entries,
> +};
> +
> /**
> * devfreq_init() - Initialize data structure for devfreq framework and
> * start polling registered devfreq devices.
> --
> 1.7.4.1
>
>
^ permalink raw reply
* Re: [PATCH pm-freezer 2/4] freezer: set PF_NOFREEZE on a dying task right before TASK_DEAD
From: Oleg Nesterov @ 2011-08-29 18:02 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, Paul Menage, containers, linux-pm
In-Reply-To: <20110829140509.GC18871@mtj.dyndns.org>
On 08/29, Tejun Heo wrote:
>
> There's no try_to_freeze() call in the exit path and the only
> necessary guarantee is that freezer doesn't hang waiting for zombies.
> Set PF_NOFREEZE right before setting tsk->state to TASK_DEAD instead.
Agreed.
But I'd like to repeat, this looks "asymmetrical". do_each_thread()
can't see the (auto)reaped tasks after they do exit_notify(). So we
can only see this PF_NOFREEZE if the thread becomes a zombie.
> @@ -1044,6 +1038,10 @@ NORET_TYPE void do_exit(long code)
>
> preempt_disable();
> exit_rcu();
> +
> + /* this task is now dead and freezer should ignore it */
> + current->flags |= PF_NOFREEZE;
> +
> /* causes final put_task_struct in finish_task_switch(). */
> tsk->state = TASK_DEAD;
May be freezing_slow_path() can check TASK_DEAD along with PF_NOFREEZE
instead? (or tsk->exit_state != 0 to avoid the asymmetry above). Just
to keep this logic in the freezer code. I dunno.
But this all is up to you and Rafael, I am not arguing. Just random
thoughts.
Oleg.
^ permalink raw reply
* Re: [PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state
From: Oleg Nesterov @ 2011-08-29 17:21 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, Paul Menage, containers, linux-pm
In-Reply-To: <20110829171228.GA11339@redhat.com>
On 08/29, Oleg Nesterov wrote:
>
> Hmm. But I got lost a bit. Why do we need freezer_lock to set/clear
> PF_FROZEN ? OK, the code below takes freezer_lock for freezing().
> Is there any other reason?
Ah, at least we need it to serialize with __thaw_task() which does
"if (frozen) wake_up()".
Oleg.
^ permalink raw reply
* Re: [PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state
From: Oleg Nesterov @ 2011-08-29 17:12 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, Paul Menage, containers, linux-pm
In-Reply-To: <20110829163533.GB9973@redhat.com>
On 08/29, Oleg Nesterov wrote:
>
> On 08/29, Tejun Heo wrote:
> >
> > --- work.orig/kernel/freezer.c
> > +++ work/kernel/freezer.c
> > @@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop
> > */
> > spin_lock_irq(&freezer_lock);
> > current->flags |= PF_FROZEN;
> > +refreeze:
> > spin_unlock_irq(&freezer_lock);
> >
> > save = current->state;
> > @@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop
> > schedule();
> > }
> >
> > - /* leave FROZEN */
> > + /* leave FROZEN after checking freezing() holding freezer_lock */
> > spin_lock_irq(&freezer_lock);
> > + if (freezing(current))
> > + goto refreeze;
>
> Looks like, you should move "save = current->state" up then.
Hmm. And afaics there is another problem. This can "livelock" if
check_kthr_stop && kthread_should_stop().
May be we should consolidate the freezer_lock's sections, something
like below?
Hmm. But I got lost a bit. Why do we need freezer_lock to set/clear
PF_FROZEN ? OK, the code below takes freezer_lock for freezing().
Is there any other reason?
Oleg.
bool __refrigerator(bool check_kthr_stop)
{
/* Hmm, should we be allowed to suspend when there are realtime
processes around? */
bool was_frozen = false;
long save;
save = current->state;
pr_debug("%s entered refrigerator\n", current->comm);
for (;;) {
set_current_state(TASK_UNINTERRUPTIBLE);
spin_lock_irq(&freezer_lock);
current->flags |= PF_FROZEN;
if (!freezing(current) ||
(check_kthr_stop && kthread_should_stop()))
current->flags &= ~PF_FROZEN;
spin_unlock_irq(&freezer_lock);
if (!current->flags & PF_FROZEN)
break;
was_frozen = true;
schedule();
}
spin_lock_irq(¤t->sighand->siglock);
recalc_sigpending(); /* We sent fake signal, clean it up */
spin_unlock_irq(¤t->sighand->siglock);
pr_debug("%s left refrigerator\n", current->comm);
/*
* Restore saved task state before returning. The mb'd version
* needs to be used; otherwise, it might silently break
* synchronization which depends on ordered task state change.
*/
set_current_state(save);
return was_frozen;
}
^ permalink raw reply
* Re: [PATCH pm-freezer 4/4] freezer: use lock_task_sighand() in fake_signal_wake_up()
From: Oleg Nesterov @ 2011-08-29 16:36 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, Paul Menage, containers, linux-pm
In-Reply-To: <20110829140621.GE18871@mtj.dyndns.org>
On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/freezer.c
> +++ work/kernel/freezer.c
> @@ -103,9 +103,10 @@ static void fake_signal_wake_up(struct t
> {
> unsigned long flags;
>
> - spin_lock_irqsave(&p->sighand->siglock, flags);
> - signal_wake_up(p, 0);
> - spin_unlock_irqrestore(&p->sighand->siglock, flags);
> + if (lock_task_sighand(p, &flags)) {
> + signal_wake_up(p, 0);
> + unlock_task_sighand(p, &flags);
> + }
Yes, this looks correct.
Oleg.
^ permalink raw reply
* Re: [PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state
From: Oleg Nesterov @ 2011-08-29 16:35 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, Paul Menage, containers, linux-pm
In-Reply-To: <20110829140549.GD18871@mtj.dyndns.org>
On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/freezer.c
> +++ work/kernel/freezer.c
> @@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop
> */
> spin_lock_irq(&freezer_lock);
> current->flags |= PF_FROZEN;
> +refreeze:
> spin_unlock_irq(&freezer_lock);
>
> save = current->state;
> @@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop
> schedule();
> }
>
> - /* leave FROZEN */
> + /* leave FROZEN after checking freezing() holding freezer_lock */
> spin_lock_irq(&freezer_lock);
> + if (freezing(current))
> + goto refreeze;
Looks like, you should move "save = current->state" up then.
Oleg.
^ permalink raw reply
* Re: [PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()
From: Oleg Nesterov @ 2011-08-29 16:00 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, Paul Menage, containers, linux-pm
In-Reply-To: <20110829140418.GB18871@mtj.dyndns.org>
On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/cgroup_freezer.c
> +++ work/kernel/cgroup_freezer.c
> @@ -311,14 +311,14 @@ static int freezer_change_state(struct c
> if (goal_state == freezer->state)
> goto out;
>
> - freezer->state = goal_state;
> -
> switch (goal_state) {
> case CGROUP_THAWED:
> + freezer->state = CGROUP_THAWED;
> atomic_dec(&system_freezing_cnt);
> unfreeze_cgroup(cgroup, freezer);
> break;
> case CGROUP_FROZEN:
> + freezer->state = CGROUP_FREEZING;
At first glance, this is correct. I'll try to recheck.
But,
> atomic_inc(&system_freezing_cnt);
iiuc this becomes wrong... Suppose a user writes "FROZEN" twice,
before freezer->state becomes CGROUP_FROZEN.
I think we should actually fix the "goal_state == freezer->state"
check above.
Oleg.
^ permalink raw reply
* [PATCH v2 4/4] bq24022: Add support for the bq2407x family
From: Heiko Stübner @ 2011-08-29 15:59 UTC (permalink / raw)
To: Mark Brown, Liam Girdwood; +Cc: linux-pm, Philipp Zabel
In-Reply-To: <201108291755.15701.heiko@sntech.de>
The bq2407x family of charger ICs simply supports another machine
specific charging current above 500mA. To use this setting a
second gpio pin is used while the rest of the pins stay the same.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
Changes since v1: use dev_err instead of dev_dbg in fatal places
drivers/regulator/Kconfig | 7 ++--
drivers/regulator/bq24022.c | 65 ++++++++++++++++++++++++++++++++++---
include/linux/regulator/bq24022.h | 7 ++++
3 files changed, 71 insertions(+), 8 deletions(-)
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index c7fd2c0..546a7a6 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -67,10 +67,11 @@ config REGULATOR_USERSPACE_CONSUMER
config REGULATOR_BQ24022
tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC"
help
- This driver controls a TI bq24022 Charger attached via
- GPIOs. The provided current regulator can enable/disable
+ This driver controls a TI bq24022 or bq2407x Charger attached
+ via GPIOs. The provided current regulator can enable/disable
charging select between 100 mA and 500 mA charging current
- limit.
+ limit. The bq2407x family also supports a machine specific
+ current limit above 500 mA.
config REGULATOR_MAX1586
tristate "Maxim 1586/1587 voltage regulator"
diff --git a/drivers/regulator/bq24022.c b/drivers/regulator/bq24022.c
index f75cd07..204ed58 100644
--- a/drivers/regulator/bq24022.c
+++ b/drivers/regulator/bq24022.c
@@ -19,15 +19,20 @@
#include <linux/gpio.h>
#include <linux/regulator/bq24022.h>
#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
struct bq24022 {
struct regulator_dev *rdev;
int gpio_nce;
int gpio_iset2;
+ int gpio_2407x_en2;
int state_nce;
int state_iset2;
+ int state_2407x_en2;
+
+ int max_uA;
};
static int bq24022_set_current_limit(struct regulator_dev *rdev,
@@ -35,12 +40,30 @@ static int bq24022_set_current_limit(struct regulator_dev *rdev,
{
struct bq24022 *bq = rdev_get_drvdata(rdev);
- dev_dbg(rdev_get_dev(rdev), "setting current limit to %s mA\n",
- max_uA >= 500000 ? "500" : "100");
+ if (bq->max_uA && bq->max_uA > 500000
+ && max_uA >= bq->max_uA && bq->gpio_2407x_en2) {
+ dev_dbg(rdev_get_dev(rdev),
+ "setting current limit to %d mA\n",
+ bq->max_uA / 1000);
+ gpio_set_value(bq->gpio_2407x_en2, 1);
+ bq->state_2407x_en2 = 1;
+ gpio_set_value(bq->gpio_iset2, 0);
+ bq->state_iset2 = 0;
+ } else if (max_uA >= 100000) {
+ dev_dbg(rdev_get_dev(rdev), "setting current limit to %s mA\n",
+ max_uA >= 500000 ? "500" : "100");
+ if (bq->gpio_2407x_en2) {
+ gpio_set_value(bq->gpio_2407x_en2, 0);
+ bq->state_2407x_en2 = 0;
+ }
+ bq->state_iset2 = (max_uA >= 500000);
+ gpio_set_value(bq->gpio_iset2, bq->state_iset2);
+ } else {
+ dev_err(rdev_get_dev(rdev), "cannot set current limit below 100 mA\n");
+ return -EINVAL;
+ }
/* REVISIT: maybe return error if min_uA != 0 ? */
- bq->state_iset2 = (max_uA >= 500000);
- gpio_set_value(bq->gpio_iset2, bq->state_iset2);
return 0;
}
@@ -48,7 +71,10 @@ static int bq24022_get_current_limit(struct regulator_dev *rdev)
{
struct bq24022 *bq = rdev_get_drvdata(rdev);
- return bq->state_iset2 ? 500000 : 100000;
+ if (bq->state_2407x_en2)
+ return bq->state_iset2 ? 0 : bq->max_uA;
+ else
+ return bq->state_iset2 ? 500000 : 100000;
}
static int bq24022_enable(struct regulator_dev *rdev)
@@ -122,8 +148,25 @@ static int __init bq24022_probe(struct platform_device *pdev)
pdata->gpio_iset2);
goto err_iset2;
}
+ if (pdata->gpio_2407x_en2) {
+ ret = gpio_request(pdata->gpio_2407x_en2, "charge_mode2");
+ if (ret) {
+ dev_err(&pdev->dev, "couldn't request EN2 GPIO: %d\n",
+ pdata->gpio_2407x_en2);
+ goto err_en2;
+ }
+ }
/* set initial current to 100mA and disable regulator */
+ if (pdata->gpio_2407x_en2) {
+ ret = gpio_direction_output(pdata->gpio_2407x_en2, 0);
+ if (ret) {
+ dev_err(&pdev->dev, "couldn't set EN2 GPIO: %d\n",
+ pdata->gpio_2407x_en2);
+ goto err_reg;
+ }
+ bq->gpio_2407x_en2 = pdata->gpio_2407x_en2;
+ }
ret = gpio_direction_output(pdata->gpio_iset2, 0);
if (ret) {
dev_err(&pdev->dev, "couldn't set ISET2 GPIO: %d\n",
@@ -140,6 +183,13 @@ static int __init bq24022_probe(struct platform_device *pdev)
bq->gpio_nce = pdata->gpio_nce;
bq->state_nce = 1;
+ /* get maximum current from regulator_init_data */
+ if (pdata->init_data) {
+ bq->max_uA = pdata->init_data->constraints.max_uA;
+ dev_dbg(&pdev->dev, "maximum current is %d mA\n",
+ bq->max_uA / 1000);
+ }
+
bq->rdev = regulator_register(&bq24022_desc, &pdev->dev,
pdata->init_data, bq);
if (IS_ERR(bq->rdev)) {
@@ -152,6 +202,9 @@ static int __init bq24022_probe(struct platform_device *pdev)
return 0;
err_reg:
+ if (pdata->gpio_2407x_en2)
+ gpio_free(pdata->gpio_2407x_en2);
+err_en2:
gpio_free(pdata->gpio_iset2);
err_iset2:
gpio_free(pdata->gpio_nce);
@@ -164,6 +217,8 @@ static int __devexit bq24022_remove(struct platform_device *pdev)
struct bq24022 *bq = platform_get_drvdata(pdev);
regulator_unregister(bq->rdev);
+ if (bq->gpio_2407x_en2)
+ gpio_free(bq->gpio_2407x_en2);
gpio_free(bq->gpio_iset2);
gpio_free(bq->gpio_nce);
diff --git a/include/linux/regulator/bq24022.h b/include/linux/regulator/bq24022.h
index a6d0140..39a48e1 100644
--- a/include/linux/regulator/bq24022.h
+++ b/include/linux/regulator/bq24022.h
@@ -16,9 +16,16 @@ struct regulator_init_data;
* bq24022_mach_info - platform data for bq24022
* @gpio_nce: GPIO line connected to the nCE pin, used to enable / disable charging
* @gpio_iset2: GPIO line connected to the ISET2 pin, used to limit charging current to 100 mA / 500 mA
+ * @gpio_2407x_en2: GPIO line connected to the en2 pin of the bq2407x family
+ * Modes of operation for bq2407x:
+ * EN2 = 0, ISET2 = 0: 100mA
+ * EN2 = 0, ISET2 = 1: 500mA
+ * EN2 = 1, ISET2 = 0: max_current
+ * EN2 = 1, ISET2 = 1: Standby (usb suspend)
*/
struct bq24022_mach_info {
int gpio_nce;
int gpio_iset2;
+ int gpio_2407x_en2;
struct regulator_init_data *init_data;
};
--
1.7.2.3
^ permalink raw reply related
* [PATCH 3/4] bq24022: Keep track of gpio states
From: Heiko Stübner @ 2011-08-29 15:58 UTC (permalink / raw)
To: Mark Brown, Liam Girdwood; +Cc: linux-pm, Philipp Zabel
In-Reply-To: <201108291755.15701.heiko@sntech.de>
gpio_get_value is not definied for output-gpios and can therefore not be
used for the is_enabled and get_current_limit methods of the bq24022.
This patch introduces a private struct to keep track of the values set.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
drivers/regulator/bq24022.c | 67 +++++++++++++++++++++++++++++-------------
1 files changed, 46 insertions(+), 21 deletions(-)
diff --git a/drivers/regulator/bq24022.c b/drivers/regulator/bq24022.c
index 7bd906c..f75cd07 100644
--- a/drivers/regulator/bq24022.c
+++ b/drivers/regulator/bq24022.c
@@ -15,56 +15,69 @@
#include <linux/platform_device.h>
#include <linux/err.h>
#include <linux/module.h>
+#include <linux/slab.h>
#include <linux/gpio.h>
#include <linux/regulator/bq24022.h>
#include <linux/regulator/driver.h>
+struct bq24022 {
+ struct regulator_dev *rdev;
+
+ int gpio_nce;
+ int gpio_iset2;
+
+ int state_nce;
+ int state_iset2;
+};
static int bq24022_set_current_limit(struct regulator_dev *rdev,
int min_uA, int max_uA)
{
- struct bq24022_mach_info *pdata = rdev_get_drvdata(rdev);
+ struct bq24022 *bq = rdev_get_drvdata(rdev);
dev_dbg(rdev_get_dev(rdev), "setting current limit to %s mA\n",
max_uA >= 500000 ? "500" : "100");
/* REVISIT: maybe return error if min_uA != 0 ? */
- gpio_set_value(pdata->gpio_iset2, max_uA >= 500000);
+ bq->state_iset2 = (max_uA >= 500000);
+ gpio_set_value(bq->gpio_iset2, bq->state_iset2);
return 0;
}
static int bq24022_get_current_limit(struct regulator_dev *rdev)
{
- struct bq24022_mach_info *pdata = rdev_get_drvdata(rdev);
+ struct bq24022 *bq = rdev_get_drvdata(rdev);
- return gpio_get_value(pdata->gpio_iset2) ? 500000 : 100000;
+ return bq->state_iset2 ? 500000 : 100000;
}
static int bq24022_enable(struct regulator_dev *rdev)
{
- struct bq24022_mach_info *pdata = rdev_get_drvdata(rdev);
+ struct bq24022 *bq = rdev_get_drvdata(rdev);
dev_dbg(rdev_get_dev(rdev), "enabling charger\n");
- gpio_set_value(pdata->gpio_nce, 0);
+ gpio_set_value(bq->gpio_nce, 0);
+ bq->state_nce = 0;
return 0;
}
static int bq24022_disable(struct regulator_dev *rdev)
{
- struct bq24022_mach_info *pdata = rdev_get_drvdata(rdev);
+ struct bq24022 *bq = rdev_get_drvdata(rdev);
dev_dbg(rdev_get_dev(rdev), "disabling charger\n");
- gpio_set_value(pdata->gpio_nce, 1);
+ gpio_set_value(bq->gpio_nce, 1);
+ bq->state_nce = 1;
return 0;
}
static int bq24022_is_enabled(struct regulator_dev *rdev)
{
- struct bq24022_mach_info *pdata = rdev_get_drvdata(rdev);
+ struct bq24022 *bq = rdev_get_drvdata(rdev);
- return !gpio_get_value(pdata->gpio_nce);
+ return !bq->state_nce;
}
static struct regulator_ops bq24022_ops = {
@@ -85,12 +98,18 @@ static struct regulator_desc bq24022_desc = {
static int __init bq24022_probe(struct platform_device *pdev)
{
struct bq24022_mach_info *pdata = pdev->dev.platform_data;
- struct regulator_dev *bq24022;
+ struct bq24022 *bq;
int ret;
if (!pdata || !pdata->gpio_nce || !pdata->gpio_iset2)
return -EINVAL;
+ bq = kzalloc(sizeof(struct bq24022), GFP_KERNEL);
+ if (!bq) {
+ dev_err(&pdev->dev, "cannot allocate memory\n");
+ return -ENOMEM;
+ }
+
ret = gpio_request(pdata->gpio_nce, "ncharge_en");
if (ret) {
dev_err(&pdev->dev, "couldn't request nCE GPIO: %d\n",
@@ -103,27 +122,32 @@ static int __init bq24022_probe(struct platform_device *pdev)
pdata->gpio_iset2);
goto err_iset2;
}
+
+ /* set initial current to 100mA and disable regulator */
ret = gpio_direction_output(pdata->gpio_iset2, 0);
if (ret) {
dev_err(&pdev->dev, "couldn't set ISET2 GPIO: %d\n",
pdata->gpio_iset2);
goto err_reg;
}
+ bq->gpio_iset2 = pdata->gpio_iset2;
ret = gpio_direction_output(pdata->gpio_nce, 1);
if (ret) {
dev_err(&pdev->dev, "couldn't set nCE GPIO: %d\n",
pdata->gpio_nce);
goto err_reg;
}
+ bq->gpio_nce = pdata->gpio_nce;
+ bq->state_nce = 1;
- bq24022 = regulator_register(&bq24022_desc, &pdev->dev,
- pdata->init_data, pdata);
- if (IS_ERR(bq24022)) {
+ bq->rdev = regulator_register(&bq24022_desc, &pdev->dev,
+ pdata->init_data, bq);
+ if (IS_ERR(bq->rdev)) {
dev_err(&pdev->dev, "couldn't register regulator\n");
- ret = PTR_ERR(bq24022);
+ ret = PTR_ERR(bq->rdev);
goto err_reg;
}
- platform_set_drvdata(pdev, bq24022);
+ platform_set_drvdata(pdev, bq);
dev_dbg(&pdev->dev, "registered regulator\n");
return 0;
@@ -137,12 +161,13 @@ err_ce:
static int __devexit bq24022_remove(struct platform_device *pdev)
{
- struct bq24022_mach_info *pdata = pdev->dev.platform_data;
- struct regulator_dev *bq24022 = platform_get_drvdata(pdev);
+ struct bq24022 *bq = platform_get_drvdata(pdev);
- regulator_unregister(bq24022);
- gpio_free(pdata->gpio_iset2);
- gpio_free(pdata->gpio_nce);
+ regulator_unregister(bq->rdev);
+ gpio_free(bq->gpio_iset2);
+ gpio_free(bq->gpio_nce);
+
+ kfree(bq);
return 0;
}
--
1.7.2.3
^ permalink raw reply related
* [PATCH 2/4] bq24022: Use dev_err instead of dev_dbg for error messages
From: Heiko Stübner @ 2011-08-29 15:57 UTC (permalink / raw)
To: Mark Brown, Liam Girdwood; +Cc: linux-pm, Philipp Zabel
In-Reply-To: <201108291755.15701.heiko@sntech.de>
This makes error messages visible to the user,
so he/she can eventually fix them.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
drivers/regulator/bq24022.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/regulator/bq24022.c b/drivers/regulator/bq24022.c
index 56627d7..7bd906c 100644
--- a/drivers/regulator/bq24022.c
+++ b/drivers/regulator/bq24022.c
@@ -93,13 +93,13 @@ static int __init bq24022_probe(struct platform_device *pdev)
ret = gpio_request(pdata->gpio_nce, "ncharge_en");
if (ret) {
- dev_dbg(&pdev->dev, "couldn't request nCE GPIO: %d\n",
+ dev_err(&pdev->dev, "couldn't request nCE GPIO: %d\n",
pdata->gpio_nce);
goto err_ce;
}
ret = gpio_request(pdata->gpio_iset2, "charge_mode");
if (ret) {
- dev_dbg(&pdev->dev, "couldn't request ISET2 GPIO: %d\n",
+ dev_err(&pdev->dev, "couldn't request ISET2 GPIO: %d\n",
pdata->gpio_iset2);
goto err_iset2;
}
@@ -119,7 +119,7 @@ static int __init bq24022_probe(struct platform_device *pdev)
bq24022 = regulator_register(&bq24022_desc, &pdev->dev,
pdata->init_data, pdata);
if (IS_ERR(bq24022)) {
- dev_dbg(&pdev->dev, "couldn't register regulator\n");
+ dev_err(&pdev->dev, "couldn't register regulator\n");
ret = PTR_ERR(bq24022);
goto err_reg;
}
--
1.7.2.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox